feat: PRD View - Document Creation & Discovery (#330)#337
Conversation
Foundation for PRD View implementation: TypeScript types mirroring prd_v2.py and discovery_v2.py Pydantic models, plus prdApi and discoveryApi client methods following the existing axios namespace pattern. Includes implementation plan document.
Renders /prd page with workspace-aware data fetching, empty state with Upload/Discovery CTAs, and PRD content preview when loaded. Action handlers are stubs for later steps.
…330) - PRDHeader: title, metadata, action buttons (Upload, Discovery, Generate Tasks) - UploadPRDModal: tabbed dialog with paste-markdown and file-upload modes, calls prdApi.create() and updates SWR cache on success - Shadcn Dialog and Tabs UI primitives (Radix-based) - Wire upload flow end-to-end in PrdPage
… save (#330) Split-view markdown editor using react-markdown for live preview. Edit tab has monospace textarea, preview tab renders markdown with prose styling. Save creates a new PRD version via prdApi.createVersion() with an optional change summary. SWR cache updated on save.
- DiscoveryTranscript: chat-style messages with AI avatar, animated thinking indicator, auto-scroll to latest message - DiscoveryInput: textarea with Ctrl+Enter send, disabled during thinking - DiscoveryPanel: manages full session lifecycle — start, answer loop, PRD generation — with error handling and loading states - PRDView: responsive two-column grid when discovery is open, single column otherwise. Placeholder shown during discovery if no PRD yet. - PrdPage: discovery open/close state, onPrdGenerated closes panel and updates SWR cache
Step 6: Replace placeholder task counts with AssociatedTasksSummary component using Badge variants, wire Generate Tasks button to discoveryApi.generateTasks() with loading state, and refresh task counts via SWR revalidation.
Step 7: Create useEventSource (generic SSE hook with auto-reconnect
and exponential backoff) and useTaskStream (typed wrapper for the
task execution SSE endpoint at /api/v2/tasks/{id}/stream). Event
types mirror the backend ExecutionEvent models.
Step 8: Create AppSidebar with icon+label navigation per Phase 3 architecture. Sidebar lists all 6 golden-path views (Workspace, PRD, Tasks, Execution, Blockers, Review) with active-route highlighting. Auto-hides when no workspace is selected. Collapses to icon-only on narrow viewports. Remove redundant breadcrumb from PRD page.
…#330) Step 9: Component tests for PRDHeader, AssociatedTasksSummary, MarkdownEditor, DiscoveryTranscript, DiscoveryInput, AppSidebar. Hook tests for useEventSource (connection lifecycle, reconnect, cleanup) and useTaskStream (event parsing, typed dispatch). Update Hugeicons mock with new PRD icons. All 152 tests pass.
WalkthroughAdds a complete PRD View feature: UI components (PRD page, header, editor, discovery panel, upload modal, task summary), SSE streaming hooks, API clients/types for PRD & discovery, Radix UI wrappers, layout/sidebar integration, comprehensive unit tests, docs, and mocks for icons. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Client as Browser / PRDView
participant API as Backend API
participant SSE as EventSource
User->>Client: Click "Start Discovery"
Client->>API: POST /discovery/start (workspacePath)
API-->>Client: StartDiscoveryResponse { sessionId, initial_question }
Client->>SSE: connect to /discovery/stream?sessionId=...
SSE-->>Client: open / stream events
loop discovery interaction
Client->>Client: render assistant question
User->>Client: Submit answer (DiscoveryInput)
Client->>API: POST /discovery/submitAnswer (sessionId, answer)
API-->>SSE: stream AI response events
SSE-->>Client: output / progress / thinking events
Client->>Client: append AI messages to Transcript
end
User->>Client: Click "Generate PRD"
Client->>API: POST /discovery/generatePrd (sessionId)
API-->>SSE: stream generation progress/completion
SSE-->>Client: generated PRD content
Client->>MarkdownEditor: populate content
User/Client->>API: POST /discovery/generateTasks (workspacePath)
API-->>Client: GenerateTasksResponse (task counts)
Client->>AssociatedTasksSummary: display counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add PRD View to support document creation and discovery by implementing
|
PR Review: PRD View - Document Creation & Discovery (#330)Overall Assessment ✅This is an excellent implementation that successfully delivers the PRD View with comprehensive testing and clean architecture. The code follows established patterns, integrates well with existing Phase 2 APIs, and provides a solid foundation for the Phase 3 UI rebuild. Summary:
Strengths1. Architecture & Organization
2. SSE Implementation (
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
- Add catch blocks to handleSavePrd and handleGenerateTasks to prevent unhandled promise rejections - Replace useState() side-effect with useEffect() for discovery session auto-start (correct React lifecycle) - Remove unused genResp variable in DiscoveryPanel - Add code review report
Updated Review - High Priority Issue Fixed ✅Status: Ready to MergeI've reviewed the current state of the PR and can confirm that the critical issue from my previous review has been addressed: ✅ Fixed: DiscoveryPanel.tsx lifecycle hook (Previous issue #1)
Current State AssessmentWhat's Been FixedThe PR now properly implements the discovery panel lifecycle with:
Remaining ObservationsThe other points from my previous review remain as post-merge enhancements (not blockers): Low priority improvements for future PRs:
None of these are blocking issues - they're architectural suggestions for incremental improvement. Final Recommendation✅ APPROVE - Ready to Merge This PR successfully delivers:
The implementation is production-ready. The remaining suggestions are nice-to-haves that can be addressed in follow-up PRs as the Phase 3 UI evolves. Manual Testing Checklist (from PR description):
Great work on establishing the SSE patterns and PRD workflow! 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Fix all issues with AI agents
In `@docs/ISSUE_330_PRD_VIEW_PLAN.md`:
- Around line 49-75: The markdown tables under the PRD (`/api/v2/prd`),
Discovery (`/api/v2/discovery`), and Tasks (`/api/v2/tasks`) sections are
missing surrounding blank lines required by MD058; fix by adding an empty line
before each table header (the lines starting with "| Method | Path | Purpose |")
and an empty line after each table block so there is a blank line both above and
below each table, ensuring the PRD, Discovery, and Tasks tables comply with
markdownlint.
- Around line 304-333: The fenced code block under the "## File Structure (all
under `web-ui/`)" section is missing a language tag (MD040); update the opening
fence from ``` to ```text so the block becomes ```text\nweb-ui/src/\n├── app/
... to satisfy the linter. Ensure the same change is applied to any similar
untagged fences in the document (search for the block starting with
"web-ui/src/").
- Around line 21-29: Add a short alignment note in the Approach (and/or
Observations) section stating how the PRD View work maps to docs/GOLDEN_PATH.md
and confirming CLI-first/headless/core-first parity; mention that the PRD View
route and discovery session must be implementable via CLI/headless flows and
that no UI-only behavior will violate the Golden Path constraints referenced in
PHASE_3_UI_ARCHITECTURE (Section 3.2). Explicitly call out expected parity
(e.g., feature availability via CLI or API endpoints, and that v2 API
integration supports headless use) and cite GOLDEN_PATH as the authoritative
guide for acceptance. Ensure this note appears near the existing
"Component-first development" / "API integration" bullet points so reviewers can
see the alignment.
In `@web-ui/src/app/prd/page.tsx`:
- Around line 118-127: The handleGenerateTasks function lacks error handling
around the async call to discoveryApi.generateTasks, so failures provide no user
feedback; wrap the await discoveryApi.generateTasks(workspacePath) and
subsequent await mutateTasks() in a try/catch, log or surface the error via your
existing UI error mechanism (e.g. processLogger or a toast/error state), and
ensure setIsGeneratingTasks(false) still runs in finally; update
handleGenerateTasks to catch the error, set an error state or show a message,
and keep the mutateTasks call only on success.
- Around line 89-103: handleSavePrd currently uses try/finally to reset
setIsSaving but does not catch errors from prdApi.createVersion so failures are
silent; wrap the await prdApi.createVersion(...) call in a try/catch (or convert
the outer try to try/catch/finally), and in the catch block log the error and
surface it to the user (e.g., set an error state, call a toast/notification, or
pass the error to mutatePrd as appropriate) while still ensuring
setIsSaving(false) runs in finally; reference function handleSavePrd and the
prdApi.createVersion call when making the change.
In `@web-ui/src/components/layout/AppLayout.tsx`:
- Around line 3-7: The props type AppLayoutProps uses React.ReactNode but the
React namespace isn't imported under the current JSX runtime; fix by importing
the type ReactNode and updating the import: add an import for ReactNode (type)
and use it in AppLayoutProps (reference: AppLayoutProps and React.ReactNode) so
the type resolves correctly without relying on the global React namespace.
In `@web-ui/src/components/layout/AppSidebar.tsx`:
- Around line 36-47: AppSidebar's hasWorkspace state doesn't update for same-tab
localStorage changes because the storage event only fires across tabs; update
setSelectedWorkspacePath in workspace-storage.ts to dispatch a custom event
(e.g., new CustomEvent('workspaceChanged')) after updating localStorage, and
modify AppSidebar (useEffect where handleStorage is added) to listen for that
custom 'workspaceChanged' event in addition to the existing 'storage' listener,
calling setHasWorkspace(!!getSelectedWorkspacePath()) in the handler so
hasWorkspace updates immediately when setSelectedWorkspacePath() is called in
the same tab.
In `@web-ui/src/components/prd/AssociatedTasksSummary.tsx`:
- Around line 11-45: The STATUS_CONFIG array used by AssociatedTasksSummary is
missing the 'MERGED' key from TaskStatusCounts, so merged tasks are counted in
total but never get a Badge; add an entry { key: 'MERGED', label: 'Merged',
variant: 'merged' } (or whichever BadgeProps['variant'] name your Badge
supports) to STATUS_CONFIG and confirm the Badge component accepts the 'merged'
variant (update Badge variant union/type if necessary) so MERGED tasks render
like the other statuses.
In `@web-ui/src/components/prd/DiscoveryPanel.tsx`:
- Around line 67-70: The code is using useState with a callback to perform a
side effect; replace that with a useEffect that runs on mount (or when sessionId
changes) to call startSession if no session exists. Locate the DiscoveryPanel
component where useState is currently invoked with a callback and switch to
React's useEffect, referencing sessionId and startSession inside the effect and
adding the appropriate dependency array (e.g., [] or [sessionId]) so the session
auto-start runs at the intended time.
In `@web-ui/src/components/prd/MarkdownEditor.tsx`:
- Around line 16-33: The draft state isn't resynced when the content prop
(initialContent) changes, causing stale editor content and incorrect hasChanges;
add a useEffect inside MarkdownEditor that watches initialContent and, when it
changes, calls setDraft(initialContent) and setChangeSummary('') to reset the
editor and summary; ensure the effect depends on initialContent so it runs on
prop updates and leaves existing handlers (handleSave, hasChanges) unchanged.
In `@web-ui/src/components/prd/PRDView.tsx`:
- Around line 104-113: The prop onSavePrd on the PRDView component should be
made required: update the PRDViewProps interface to change onSavePrd? to
onSavePrd, and then remove the no-op fallback used when rendering MarkdownEditor
(remove the "?? (async () => {})" fallback) so that MarkdownEditor always
receives the real onSave handler; locate the PRDView component and its
PRDViewProps type and update usage where MarkdownEditor is rendered to pass
onSave={onSavePrd} directly.
🧹 Nitpick comments (9)
web-ui/__tests__/components/prd/MarkdownEditor.test.tsx (1)
91-97: Consider enhancingisSavingstate test coverage.The current test only verifies that the component doesn't crash when
isSaving=truewithout content changes. The inline comment explains the limitation, but consider adding a test that modifies content first, then verifies the save button shows a loading state or is disabled whenisSavingis true.💡 Optional enhancement
it('shows saving state when content modified and isSaving is true', async () => { const user = userEvent.setup(); const { rerender } = render(<MarkdownEditor content="original" onSave={jest.fn()} />); const textarea = screen.getByRole('textbox'); await user.type(textarea, ' extra'); rerender(<MarkdownEditor content="original" onSave={jest.fn()} isSaving />); const saveButton = screen.getByRole('button', { name: /save/i }); expect(saveButton).toBeDisabled(); });web-ui/__tests__/hooks/useTaskStream.test.ts (1)
9-14: Mock returns static'open'status regardless of URL.The mock always returns
status: 'open', but whentaskIdisnull,useEventSourcewould receiveurl: nulland should returnstatus: 'idle'. This means the test at lines 22-27 doesn't fully validate the idle behavior.💡 Optional: Dynamic status based on URL
jest.mock('@/hooks/useEventSource', () => ({ - useEventSource: ({ onMessage }: { url: string | null; onMessage?: (data: string) => void }) => { + useEventSource: ({ url, onMessage }: { url: string | null; onMessage?: (data: string) => void }) => { capturedOnMessage = onMessage; - return { status: 'open' as const, close: mockClose }; + return { status: url ? 'open' as const : 'idle' as const, close: mockClose }; }, }));web-ui/src/app/prd/page.tsx (1)
22-24: useEffect runs only on mount; workspace changes won't update.The empty dependency array means workspace path is only read once on mount. If the user changes workspace in another tab or component, this page won't reflect the change until a full page reload.
💡 Optional: Listen for storage events
useEffect(() => { setWorkspacePath(getSelectedWorkspacePath()); + + const handleStorageChange = () => { + setWorkspacePath(getSelectedWorkspacePath()); + }; + window.addEventListener('storage', handleStorageChange); + return () => window.removeEventListener('storage', handleStorageChange); }, []);web-ui/__tests__/components/layout/AppSidebar.test.tsx (1)
98-106: Complex regex assertion for inactive routes may be fragile.The regex
(?<!\w)bg-accent(?!\/)with negative lookbehind is clever but may break if class naming conventions change. Consider a more explicit assertion.💡 Alternative assertion approach
it('does not highlight inactive routes', () => { mockGetWorkspacePath.mockReturnValue('/home/user/projects/test'); mockUsePathname.mockReturnValue('/prd'); render(<AppSidebar />); const workspaceLink = screen.getByRole('link', { name: /workspace/i }); // More explicit: active class is 'bg-accent', inactive uses 'hover:bg-accent/50' expect(workspaceLink).not.toHaveClass('bg-accent'); // Or check it has the hover variant instead expect(workspaceLink.className).toContain('hover:bg-accent'); });web-ui/__tests__/hooks/useEventSource.test.ts (1)
139-162: Consider verifying exponential backoff timing.The test verifies a retry occurs after the initial delay (100ms), but doesn't confirm the exponential multiplier for subsequent retries. If the hook uses exponential backoff (e.g., 100ms → 200ms → 400ms), consider verifying the second retry delay is doubled.
💡 Optional: Verify exponential multiplier
it('uses exponential backoff for retries', () => { const { result } = renderHook(() => useEventSource({ url: '/api/stream', maxRetries: 3, retryDelay: 100, }) ); // First error → schedule retry at 100ms act(() => { MockEventSource.latest().simulateError(true); }); act(() => { jest.advanceTimersByTime(100); }); expect(MockEventSource._instances).toHaveLength(2); // Second error → should retry at 200ms (100 * 2) act(() => { MockEventSource.latest().simulateError(true); }); act(() => { jest.advanceTimersByTime(100); // Only 100ms passed }); expect(MockEventSource._instances).toHaveLength(2); // No new instance yet act(() => { jest.advanceTimersByTime(100); // Total 200ms }); expect(MockEventSource._instances).toHaveLength(3); // Now retried });web-ui/src/types/prd.ts (1)
50-54: Consider stronger typing forRecord<string, unknown>fields when schemas stabilize.Fields like
question,next_question,current_question,progress,coverage, andtasksuseRecord<string, unknown>, which sacrifices type safety. Once the backend Pydantic models stabilize, consider defining explicit interfaces for these to improve autocomplete and catch type errors at compile time.Also applies to: 61-62, 68-69, 81-81
web-ui/src/hooks/useEventSource.ts (1)
96-114: Consider exposing retry state or adding anonMaxRetriesExceededcallback.When retries exhaust (
retriesRef.current >= maxRetries), the status transitions to'error'but the consumer has no direct notification beyond pollingstatus. For observability, you could optionally expose a callback or the current retry count.web-ui/src/hooks/useTaskStream.ts (2)
111-141: Minor: Consider memoizing callbacks passed to this hook.The
handleMessagecallback depends on all event callbacks in its dependency array (Line 140). If consumers pass inline arrow functions,handleMessagewill be recreated each render. WhileuseEventSourcemitigates this via refs, documenting the recommendation to wrap callbacks inuseCallbackwould help consumers avoid unnecessary work.
119-133: Redundant type assertions after discriminant narrowing.After switching on
event.event_type, TypeScript narrows the type automatically. The explicit casts (e.g.,event as ProgressEvent) are not required and can be removed for cleaner code.♻️ Suggested simplification
switch (event.event_type) { case 'progress': - onProgress?.(event as ProgressEvent); + onProgress?.(event); break; case 'output': - onOutput?.(event as OutputEvent); + onOutput?.(event); break; case 'blocker': - onBlocker?.(event as BlockerEvent); + onBlocker?.(event); break; case 'completion': - onComplete?.(event as CompletionEvent); + onComplete?.(event); break; case 'error': - onError?.(event as ErrorEvent); + onError?.(event); break;
| useEffect(() => { | ||
| setHasWorkspace(!!getSelectedWorkspacePath()); | ||
| }, []); | ||
|
|
||
| // Listen for storage changes (workspace selection/deselection) | ||
| useEffect(() => { | ||
| const handleStorage = () => { | ||
| setHasWorkspace(!!getSelectedWorkspacePath()); | ||
| }; | ||
| window.addEventListener('storage', handleStorage); | ||
| return () => window.removeEventListener('storage', handleStorage); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for workspace-related keys and functions
rg -n "workspace-storage|getSelectedWorkspacePath|setSelectedWorkspacePath|STORAGE_KEY" -S --type=ts --type=tsxRepository: frankbria/codeframe
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search for localStorage.setItem and removeItem calls
rg -n "localStorage\\.setItem\\(|localStorage\\.removeItem\\(" -S --type=ts --type=tsxRepository: frankbria/codeframe
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Also search for window.dispatchEvent or custom events related to workspace
rg -n "dispatchEvent|CustomEvent.*workspace" -S --type=ts --type=tsxRepository: frankbria/codeframe
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search for workspace-related patterns without type specification
rg "workspace-storage|getSelectedWorkspacePath|setSelectedWorkspacePath|STORAGE_KEY" -nRepository: frankbria/codeframe
Length of output: 3477
🏁 Script executed:
#!/bin/bash
# Search for localStorage operations
rg "localStorage\.(setItem|removeItem)" -nRepository: frankbria/codeframe
Length of output: 1468
🏁 Script executed:
#!/bin/bash
# Find the file structure to understand the codebase
find . -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: frankbria/codeframe
Length of output: 1257
🏁 Script executed:
#!/bin/bash
# Read the workspace-storage.ts file to see if custom events are dispatched
cat -n web-ui/src/lib/workspace-storage.tsRepository: frankbria/codeframe
Length of output: 3139
🏁 Script executed:
#!/bin/bash
# Check where setSelectedWorkspacePath is called in page.tsx (around lines 145-150)
sed -n '130,160p' web-ui/src/app/page.tsxRepository: frankbria/codeframe
Length of output: 999
🏁 Script executed:
#!/bin/bash
# Read the entire AppSidebar.tsx file
cat -n web-ui/src/components/layout/AppSidebar.tsxRepository: frankbria/codeframe
Length of output: 4057
🏁 Script executed:
#!/bin/bash
# Check if there are any other event listeners or state management in AppSidebar
rg "useEffect|useState|useCallback|dispatchEvent|CustomEvent" web-ui/src/components/layout/AppSidebar.tsx -nRepository: frankbria/codeframe
Length of output: 217
AppSidebar doesn't update when workspace is selected in the same tab.
The storage event listener (lines 41-47) won't fire for localStorage changes made in the same tab. When setSelectedWorkspacePath() is called from page.tsx, AppSidebar's hasWorkspace state remains stale. Dispatch a custom event from setSelectedWorkspacePath() in workspace-storage.ts and listen for it in AppSidebar, or use a shared store mechanism like useSyncExternalStore.
🤖 Prompt for AI Agents
In `@web-ui/src/components/layout/AppSidebar.tsx` around lines 36 - 47,
AppSidebar's hasWorkspace state doesn't update for same-tab localStorage changes
because the storage event only fires across tabs; update
setSelectedWorkspacePath in workspace-storage.ts to dispatch a custom event
(e.g., new CustomEvent('workspaceChanged')) after updating localStorage, and
modify AppSidebar (useEffect where handleStorage is added) to listen for that
custom 'workspaceChanged' event in addition to the existing 'storage' listener,
calling setHasWorkspace(!!getSelectedWorkspacePath()) in the handler so
hasWorkspace updates immediately when setSelectedWorkspacePath() is called in
the same tab.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/code-review/2026-02-05-prd-view-review.md`:
- Around line 1-305: The new markdown "Code Review Report: PRD View - Document
Creation & Discovery" lacks justification vs the Golden Path; either (A) update
the document to explicitly state how the PRD View aligns with GOLDEN_PATH.md and
the CLI-first/headless/core-first principles (cite specific clauses from
GOLDEN_PATH.md, describe how features like API-first endpoints, non-UI
fallbacks, and workspace state context satisfy CLI/headless/core goals, and add
a short section "Alignment with Golden Path" near the top), or (B) if it cannot
be justified, move/remove the doc per guidelines; ensure the change references
the guideline phrase "If a change does not directly support Golden Path" and
include a brief reviewer note explaining the decision.
In `@web-ui/src/app/prd/page.tsx`:
- Around line 15-60: The initial render shows "No workspace selected" because
workspacePath is null until useEffect runs; add a new state const
[workspaceReady, setWorkspaceReady] = useState(false) and in the existing
useEffect call setWorkspacePath(getSelectedWorkspacePath()) then
setWorkspaceReady(true); update the early-return guard to check if
(!workspaceReady) return null or a loading placeholder, and keep the existing
!workspacePath block only for when workspaceReady is true and no workspace is
selected (i.e., change the condition to if (workspaceReady && !workspacePath)
{...}); reference PrdPage, workspacePath, useEffect and getSelectedWorkspacePath
when making the change.
🧹 Nitpick comments (2)
web-ui/src/components/prd/DiscoveryPanel.tsx (2)
42-42: Consider extractingnow()outside the component.This helper is recreated on every render. Since it has no dependencies on component scope, it could be moved outside or inlined at call sites.
♻️ Optional refactor
+const now = () => new Date().toISOString(); + export function DiscoveryPanel({ workspacePath, onClose, onPrdGenerated, }: DiscoveryPanelProps) { const [sessionId, setSessionId] = useState<string | null>(null); // ... - const now = () => new Date().toISOString();
59-61: Consider a type guard for safer error handling.The
as ApiErrorassertion is repeated across three catch blocks. If the error is not anApiError(e.g., a networkTypeError), accessingapiErr.detailyieldsundefined, which the fallback handles—but the cast is technically unsafe.♻️ Optional: Extract a helper for consistent error handling
function getErrorMessage(err: unknown, fallback: string): string { if (err && typeof err === 'object' && 'detail' in err && typeof (err as ApiError).detail === 'string') { return (err as ApiError).detail; } return fallback; } // Usage: } catch (err) { setError(getErrorMessage(err, 'Failed to start discovery session')); }Also applies to: 108-110, 129-131
| # Code Review Report: PRD View - Document Creation & Discovery | ||
|
|
||
| **Date:** 2026-02-05 | ||
| **Reviewer:** Code Review Agent | ||
| **Component:** PRD View (PR #337, Issue #330) | ||
| **Files Reviewed:** 34 files (20 source, 8 test, 1 mock, 1 docs, 4 config) | ||
| **Ready for Production:** Yes, with 2 major issues recommended for near-term fix | ||
|
|
||
| ## Executive Summary | ||
|
|
||
| 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. | ||
|
|
||
| **Critical Issues:** 0 | ||
| **Major Issues:** 2 | ||
| **Minor Issues:** 4 | ||
| **Positive Findings:** 7 | ||
|
|
||
| --- | ||
|
|
||
| ## Review Context | ||
|
|
||
| **Code Type:** Frontend (Next.js React components, hooks, API client) | ||
| **Risk Level:** Medium (user input handling, file upload, SSE connections, AI chat rendering) | ||
| **Business Constraints:** Phase 3 UI rebuild — first user-facing view beyond workspace | ||
|
|
||
| ### Review Focus Areas | ||
|
|
||
| - ✅ A03 - Injection/XSS — User markdown rendered via react-markdown, file upload content | ||
| - ✅ Reliability — Error handling in async handlers, resource cleanup in SSE hooks | ||
| - ✅ Resource Management — EventSource lifecycle, FileReader cleanup, SWR cache management | ||
| - ✅ A06 - Vulnerable Components — Dependency audit | ||
| - ❌ OWASP LLM Top 10 — Skipped (frontend doesn't interact with LLM directly) | ||
| - ❌ Zero Trust / Auth — Skipped (auth is backend concern; API client already has `withCredentials: true`) | ||
| - ❌ Performance — Skipped (not performance-critical UI code) | ||
|
|
||
| --- | ||
|
|
||
| ## Priority 1 Issues - Critical ⛔ | ||
|
|
||
| None found. | ||
|
|
||
| --- | ||
|
|
||
| ## Priority 2 Issues - Major ⚠️ | ||
|
|
||
| ### 1. Missing error handling in `handleSavePrd` and `handleGenerateTasks` | ||
|
|
||
| **Location:** `web-ui/src/app/prd/page.tsx:89-103` and `web-ui/src/app/prd/page.tsx:118-127` | ||
| **Severity:** Major | ||
| **Category:** Reliability | ||
|
|
||
| **Problem:** | ||
| 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. | ||
|
|
||
| **Current Code:** | ||
| ```typescript | ||
| const handleSavePrd = async (content: string, changeSummary: string) => { | ||
| if (!prd || !workspacePath) return; | ||
| setIsSaving(true); | ||
| try { | ||
| const updated = await prdApi.createVersion(...); | ||
| mutatePrd(updated, false); | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; | ||
| ``` | ||
|
|
||
| **Recommended Fix:** | ||
| ```typescript | ||
| const handleSavePrd = async (content: string, changeSummary: string) => { | ||
| if (!prd || !workspacePath) return; | ||
| setIsSaving(true); | ||
| try { | ||
| const updated = await prdApi.createVersion(...); | ||
| mutatePrd(updated, false); | ||
| } catch (err) { | ||
| const apiError = err as ApiError; | ||
| console.error('[PRD] Save failed:', apiError.detail); | ||
| // TODO: Show error toast/banner to user | ||
| } finally { | ||
| setIsSaving(false); | ||
| } | ||
| }; | ||
| ``` | ||
|
|
||
| **Why This Fix Works:** | ||
| 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. | ||
|
|
||
| --- | ||
|
|
||
| ### 2. Misuse of `useState` as initializer in DiscoveryPanel | ||
|
|
||
| **Location:** `web-ui/src/components/prd/DiscoveryPanel.tsx:68-70` | ||
| **Severity:** Major | ||
| **Category:** Reliability / Correctness | ||
|
|
||
| **Problem:** | ||
| `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: | ||
| 1. It violates React's rules — side effects should use `useEffect` | ||
| 2. The async call fires during render, not after mount | ||
| 3. React StrictMode in development will call it twice | ||
|
|
||
| **Current Code:** | ||
| ```typescript | ||
| useState(() => { | ||
| if (!sessionId) startSession(); | ||
| }); | ||
| ``` | ||
|
|
||
| **Recommended Fix:** | ||
| ```typescript | ||
| useEffect(() => { | ||
| if (!sessionId) startSession(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
| ``` | ||
|
|
||
| **Why This Fix Works:** | ||
| `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). | ||
|
|
||
| --- | ||
|
|
||
| ## Priority 3 Issues - Minor 📝 | ||
|
|
||
| ### 1. `sessionId` interpolated directly into URL path | ||
|
|
||
| **Location:** `web-ui/src/lib/api.ts:258`, `270` | ||
| **Severity:** Minor | ||
| **Category:** A03 - Injection (Defense in depth) | ||
|
|
||
| **Recommendation:** | ||
| `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. | ||
|
|
||
| **Suggested Approach:** | ||
| ```typescript | ||
| `/api/v2/discovery/${encodeURIComponent(sessionId)}/answer` | ||
| ``` | ||
|
|
||
| This is a nitpick — the backend validates the session ID format, and the value originates from the server. No immediate risk. | ||
|
|
||
| --- | ||
|
|
||
| ### 2. No file size limit on upload | ||
|
|
||
| **Location:** `web-ui/src/components/prd/UploadPRDModal.tsx:45-67` | ||
| **Severity:** Minor | ||
| **Category:** Reliability | ||
|
|
||
| **Recommendation:** | ||
| 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. | ||
|
|
||
| **Suggested Approach:** | ||
| Add a size check before reading: | ||
| ```typescript | ||
| const MAX_FILE_SIZE = 5 * 1024 * 1024; // 5 MB | ||
| if (file.size > MAX_FILE_SIZE) { | ||
| setError('File too large (max 5 MB)'); | ||
| return; | ||
| } | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 3. `genResp` variable unused in `handleGeneratePrd` | ||
|
|
||
| **Location:** `web-ui/src/components/prd/DiscoveryPanel.tsx:124` | ||
| **Severity:** Minor | ||
| **Category:** Code Quality | ||
|
|
||
| **Recommendation:** | ||
| 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. | ||
|
|
||
| **Suggested Approach:** | ||
| ```typescript | ||
| await discoveryApi.generatePrd(sessionId, workspacePath); | ||
| const fullPrd = await prdApi.getLatest(workspacePath); | ||
| ``` | ||
|
|
||
| --- | ||
|
|
||
| ### 4. DiscoveryPanel and PRDView lack test coverage | ||
|
|
||
| **Location:** `web-ui/src/components/prd/DiscoveryPanel.tsx`, `PRDView.tsx` | ||
| **Severity:** Minor | ||
| **Category:** Test Coverage | ||
|
|
||
| **Recommendation:** | ||
| 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. | ||
|
|
||
| **Suggested Approach:** | ||
| Add tests that mock `@/lib/api` and SWR, then verify: | ||
| - DiscoveryPanel: session auto-start, answer submission flow, error display | ||
| - PRDView: loading/empty/content state rendering, discovery toggle | ||
|
|
||
| --- | ||
|
|
||
| ## Positive Findings ✨ | ||
|
|
||
| ### Excellent Practices | ||
| - **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. | ||
| - **SWR optimistic updates:** `mutatePrd(newPrd, false)` correctly uses `false` for `revalidate` to avoid redundant refetches after mutations. | ||
| - **Proper React patterns:** Stable callback refs in `useEventSource` prevent unnecessary effect re-runs. `useCallback` used consistently for handlers passed to children. | ||
|
|
||
| ### Good Architectural Decisions | ||
| - **Incremental commits:** Each of the 9 commits is independently reviewable and represents a testable increment — excellent for bisecting bugs. | ||
| - **Component separation:** Clear responsibility split (PRDView = layout, PRDHeader = actions, MarkdownEditor = content, DiscoveryPanel = chat lifecycle). Each component is independently testable. | ||
| - **Generic + specific hook pattern:** `useEventSource` (generic SSE) wrapping into `useTaskStream` (typed for task events) is a clean, reusable pattern. | ||
|
|
||
| ### Security Wins | ||
| - **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`. | ||
| - **`accept` attribute on file input:** Limits file picker to `.md,.markdown,.txt` — client-side defense against wrong file types. | ||
| - **API client `withCredentials: true`:** Already configured in the existing axios instance — cookies sent with cross-origin requests, matching backend auth pattern. | ||
| - **Zero npm audit vulnerabilities:** `npm audit --production` shows 0 vulnerabilities. | ||
|
|
||
| --- | ||
|
|
||
| ## Team Collaboration Needed | ||
|
|
||
| ### Handoffs to Other Agents | ||
|
|
||
| **Architecture Agent:** | ||
| - 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. | ||
|
|
||
| **UX Designer Agent:** | ||
| - 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. | ||
| - 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. | ||
|
|
||
| --- | ||
|
|
||
| ## Testing Recommendations | ||
|
|
||
| ### Unit Tests Needed | ||
| - [x] PRDHeader (10 tests) ✅ | ||
| - [x] AssociatedTasksSummary (4 tests) ✅ | ||
| - [x] MarkdownEditor (8 tests) ✅ | ||
| - [x] DiscoveryTranscript (6 tests) ✅ | ||
| - [x] DiscoveryInput (9 tests) ✅ | ||
| - [x] AppSidebar (7 tests) ✅ | ||
| - [x] useEventSource (9 tests) ✅ | ||
| - [x] useTaskStream (9 tests) ✅ | ||
| - [ ] DiscoveryPanel (mock API, test session lifecycle) | ||
| - [ ] PRDView (test state-driven rendering) | ||
| - [ ] UploadPRDModal (mock prdApi.create, test submit flow) | ||
|
|
||
| ### Integration Tests | ||
| - [ ] Full discovery flow: mount → auto-start → answer questions → generate PRD | ||
| - [ ] Upload PRD via paste → verify editor populated | ||
| - [ ] Task generation → verify AssociatedTasksSummary updates | ||
|
|
||
| --- | ||
|
|
||
| ## Future Considerations | ||
|
|
||
| ### Patterns for Project Evolution | ||
| - **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. | ||
| - **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. | ||
|
|
||
| ### Technical Debt Items | ||
| - `useState` misuse in DiscoveryPanel (issue #2 above) — should be fixed before more components copy this pattern | ||
| - Unused `genResp` variable (issue #3 above) | ||
|
|
||
| --- | ||
|
|
||
| ## Compliance & Best Practices | ||
|
|
||
| ### Security Standards Met | ||
| - ✅ No raw HTML rendering in markdown (react-markdown default config) | ||
| - ✅ File upload restricted by `accept` attribute | ||
| - ✅ API client uses `withCredentials` for cookie-based auth | ||
| - ✅ No secrets or credentials in frontend code | ||
| - ✅ Zero npm audit vulnerabilities | ||
| - ✅ User input sent to API via POST body (not URL path), except workspace_path which is from localStorage | ||
|
|
||
| ### Enterprise Best Practices | ||
| - ✅ TypeScript strict types for all API responses | ||
| - ✅ Consistent error handling pattern across components | ||
| - ✅ 64 tests with 93-100% coverage on tested components | ||
| - ⚠️ Two orchestrator components (DiscoveryPanel, PRDView) at 0% coverage | ||
|
|
||
| --- | ||
|
|
||
| ## Action Items Summary | ||
|
|
||
| ### Immediate (Before Merge - Recommended) | ||
| 1. Add `catch` blocks to `handleSavePrd` and `handleGenerateTasks` in `page.tsx` | ||
| 2. Replace `useState()` with `useEffect()` for auto-start in `DiscoveryPanel.tsx` | ||
|
|
||
| ### Short-term (Next Sprint) | ||
| 1. Add toast/notification system for error feedback | ||
| 2. Add tests for DiscoveryPanel and PRDView orchestrator components | ||
| 3. Add file size validation to UploadPRDModal | ||
|
|
||
| ### Long-term (Backlog) | ||
| 1. Workspace state context (replace localStorage reads per-component) | ||
| 2. `encodeURIComponent` for path-interpolated IDs in API client | ||
| 3. Remove unused `genResp` variable | ||
|
|
||
| --- | ||
|
|
||
| ## Conclusion | ||
|
|
||
| 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. | ||
|
|
||
| **Recommendation:** Fix the 2 major issues, then merge. Short-term items can be addressed in follow-up. |
There was a problem hiding this comment.
Document alignment with Golden Path not demonstrated.
This new markdown doc does not indicate how it directly supports the Golden Path or CLI-first/headless/core-first architecture. Please justify alignment or relocate/remove per guidelines. As per coding guidelines: "If a change does not directly support Golden Path (defined in docs/GOLDEN_PATH.md), do not implement it. Verify alignment with CLI-first, headless, core-first architecture before implementing any feature."
🧰 Tools
🪛 LanguageTool
[style] ~151-~151: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ... size. A user could accidentally select a very large file (e.g., a binary misnamed .md), c...
(EN_WEAK_ADJECTIVE)
🤖 Prompt for AI Agents
In `@docs/code-review/2026-02-05-prd-view-review.md` around lines 1 - 305, The new
markdown "Code Review Report: PRD View - Document Creation & Discovery" lacks
justification vs the Golden Path; either (A) update the document to explicitly
state how the PRD View aligns with GOLDEN_PATH.md and the
CLI-first/headless/core-first principles (cite specific clauses from
GOLDEN_PATH.md, describe how features like API-first endpoints, non-UI
fallbacks, and workspace state context satisfy CLI/headless/core goals, and add
a short section "Alignment with Golden Path" near the top), or (B) if it cannot
be justified, move/remove the doc per guidelines; ensure the change references
the guideline phrase "If a change does not directly support Golden Path" and
include a brief reviewer note explaining the decision.
#330) - Import ReactNode explicitly in AppLayout (jsx: react-jsx compat) - Dispatch custom 'workspaceChanged' event for same-tab sidebar updates - Add MERGED status to Badge variants and AssociatedTasksSummary - Sync MarkdownEditor draft when content prop changes via useEffect - Make onSavePrd required in PRDView, remove no-op fallback - Add workspaceReady guard to prevent flash of "No workspace" on mount - Remove redundant type assertions in useTaskStream switch - Extract now() helper outside DiscoveryPanel component - Abort previous FileReader on new file selection in UploadPRDModal - Reset SSE retries on first message (not on open) to prevent loops - Fix pre-existing MERGED type error in WorkspaceStatsCards test
PR Review: PRD View - Document Creation & DiscoveryOverall Assessment: ✅ Approved with minor suggestions This is an excellent implementation of the PRD View feature. The code is well-structured, follows established project patterns, and includes comprehensive test coverage. The two major issues from the initial code review (missing error handling and misused React hook) have been fixed ✅. Strengths 💪Architecture & Code Quality
Error Handling (Fixed ✅)Both major reliability issues from the code review document have been addressed:
Security & Best Practices
TestingThe PR description states 64 new tests added (152 total, all passing). This demonstrates thorough testing of individual components. Minor Suggestions 📝1. User feedback for save/generate errorsCurrent state: Errors are logged to console but not shown to the user. Recommendation: Add toast notifications for better UX. Since the project doesn't appear to have a toast system yet, this could be a follow-up task. Current console logging is acceptable for now. Files: 2. File size validation (defense in depth)Current state: No file size limit on upload. Recommendation: Add a max file size check before reading to prevent memory issues with large files. File: Priority: Low (nice-to-have, not critical) 3. URL encoding for sessionId (defense in depth)Current state: Recommendation: While the sessionId comes from the server (not user input), encoding would add defense-in-depth using Files: Priority: Low (nitpick, no immediate risk) Compliance with CLAUDE.md ✅The implementation aligns well with project guidelines:
Test Plan VerificationFrom PR description:
Recommendation: Complete manual testing checklist before merge. SummaryThis PR represents high-quality work that follows best practices and addresses all critical issues identified in the code review. The three minor suggestions above are optional enhancements that can be addressed in follow-up PRs if desired. Decision: Approve ✅ Great work on implementing the full PRD View golden path! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@web-ui/src/app/prd/page.tsx`:
- Around line 23-26: The PRD page currently only reads
getSelectedWorkspacePath() on mount; update the useEffect to subscribe to the
global "workspaceChanged" event and call
setWorkspacePath(getSelectedWorkspacePath()) (and setWorkspaceReady(true) if
needed) whenever the event fires so the page stays in sync; add a cleanup
function to remove the listener to avoid leaks. Locate the useEffect block in
page.tsx (the one calling setWorkspacePath/getSelectedWorkspacePath and
setWorkspaceReady) and implement the event listener registration and teardown
there.
In `@web-ui/src/components/prd/DiscoveryPanel.tsx`:
- Around line 67-71: The useEffect in DiscoveryPanel that auto-starts sessions
calls startSession() twice under React StrictMode because sessionId is still
falsy during the double-mount; guard against duplicate backend session creation
by adding a ref-based guard or abort flag: introduce a local mutable ref (e.g.,
startedRef) checked inside the useEffect before calling startSession(), set
startedRef.current = true immediately after calling startSession(), and clear it
in a cleanup if needed; ensure the effect still depends on sessionId (or keep
empty deps but use the ref) so startSession is only invoked once even when React
mounts twice.
🧹 Nitpick comments (4)
web-ui/src/hooks/useTaskStream.ts (1)
111-141: Consider a lightweight guard on parsed SSE payloads.
JSON.parse(data) as ExecutionEvent(Line 114) trusts the server shape at compile time only. If the backend ever sends a valid-JSON payload that lacksevent_typeortask_id, the switch silently falls through andonEventreceives a mis-typed object, potentially causing undefined-property access in consumers.A minimal runtime guard before dispatching would close this gap without pulling in a schema library:
Proposed guard
try { const event = JSON.parse(data) as ExecutionEvent; + if (!event.event_type || !event.task_id) return; // ignore malformed payloads setLastEvent(event); onEvent?.(event);web-ui/src/components/prd/DiscoveryPanel.tsx (1)
44-65: No cleanup/abort on unmount for in-flightstartSession.If the panel unmounts while
discoveryApi.startis still in flight, the resolved promise will callsetSessionId/setState/setMessageson an unmounted component. While React 19 no longer warns for this, it can still lead to a wasted backend session and a brief UI glitch if the parent re-renders.Consider using an
AbortControlleror a cleanup flag:♻️ Sketch using a cancelled flag
const startSession = useCallback(async () => { + // caller can set cancelled = true on unmount setIsThinking(true); setError(null); try { const resp = await discoveryApi.start(workspacePath); + if (cancelled) return; // <-- check before state updates setSessionId(resp.session_id); ...Wire
cancelledthrough theuseEffectcleanup return.web-ui/src/components/prd/MarkdownEditor.tsx (1)
7-7: UnusedTabsContentimport.
TabsContentis imported but never used — content is rendered conditionally viaactiveTabchecks instead.🧹 Proposed fix
-import { Tabs, TabsList, TabsTrigger, TabsContent } from '@/components/ui/tabs'; +import { Tabs, TabsList, TabsTrigger } from '@/components/ui/tabs';web-ui/src/app/prd/page.tsx (1)
96-113: Errors in save and task generation are only logged to console — no user-facing feedback.Both
handleSavePrd(Line 109) andhandleGenerateTasks(Line 136) catch errors andconsole.errorthem but don't surface anything to the user. The user's save or generate action silently fails with no indication. Consider adding a toast, inline error banner, or an error state prop passed toPRDView.Also applies to: 128-140
| useEffect(() => { | ||
| setWorkspacePath(getSelectedWorkspacePath()); | ||
| setWorkspaceReady(true); | ||
| }, []); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for workspaceChanged event listeners and workspace switching patterns
rg -n "workspaceChanged" web-ui/src/ -A 3Repository: frankbria/codeframe
Length of output: 1099
🏁 Script executed:
# Also check the workspace-storage.ts file to understand the event dispatch
fd -n workspace-storage web-ui/src/ -type fRepository: frankbria/codeframe
Length of output: 293
🏁 Script executed:
# Check the sidebar/navigation to see how workspace switching is handled
fd -n sidebar web-ui/src/ -o -name "*sidebar*" -o -name "*nav*" | head -20Repository: frankbria/codeframe
Length of output: 293
🏁 Script executed:
cat -n web-ui/src/components/layout/AppSidebar.tsx | head -70Repository: frankbria/codeframe
Length of output: 2972
🏁 Script executed:
# Check if AppSidebar navigates to routes when workspace changes
rg -n "handleWorkspaceChange" web-ui/src/components/layout/AppSidebar.tsx -A 10Repository: frankbria/codeframe
Length of output: 835
🏁 Script executed:
# Check the full AppSidebar file to understand navigation behavior
wc -l web-ui/src/components/layout/AppSidebar.tsxRepository: frankbria/codeframe
Length of output: 110
🏁 Script executed:
# Search for workspace selector UI and setSelectedWorkspacePath calls
rg -n "setSelectedWorkspacePath\|selectWorkspace" web-ui/src/ -B 2 -A 2Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Look for workspace picker/selector UI components
fd workspace web-ui/src/components/ -type fRepository: frankbria/codeframe
Length of output: 233
🏁 Script executed:
# Check what triggers workspace changes - look for onclick or link handlers
rg -n "onClick\|href" web-ui/src/components/layout/AppSidebar.tsx | tail -30Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Look for workspace selection - check home/workspace page
cat -n web-ui/src/app/page.tsx 2>/dev/null | head -100Repository: frankbria/codeframe
Length of output: 3743
🏁 Script executed:
# Search more broadly for where workspace path is set
rg -n "localStorage.*workspace" web-ui/src/ -i | head -20Repository: frankbria/codeframe
Length of output: 386
🏁 Script executed:
# Check root layout
ls -la web-ui/src/app/layout.tsxRepository: frankbria/codeframe
Length of output: 136
🏁 Script executed:
# Find and examine WorkspaceSelector component
fd -t f "WorkspaceSelector*" web-ui/src/components/Repository: frankbria/codeframe
Length of output: 116
🏁 Script executed:
# Look at the full page.tsx to see what happens when workspace is selected
cat -n web-ui/src/app/page.tsx | sed -n '100,200p'Repository: frankbria/codeframe
Length of output: 4121
🏁 Script executed:
# Check the full WorkspaceSelector to understand where workspace switching happens
wc -l web-ui/src/components/workspace/WorkspaceSelector.tsxRepository: frankbria/codeframe
Length of output: 120
🏁 Script executed:
# Check if there's workspace switching UI anywhere else besides home page
rg -n "handleSwitchWorkspace\|clearSelectedWorkspacePath" web-ui/src/ -B 2 -A 2Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Verify page.tsx /prd to see the full useEffect hooks
cat -n web-ui/src/app/prd/page.tsx | head -50Repository: frankbria/codeframe
Length of output: 1990
🏁 Script executed:
# Check if there's any workspace selector in the header or nav
rg -n "WorkspaceHeader\|workspace.*select\|select.*workspace" web-ui/src/components/ -i | head -30Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Verify the actual behavior: does changing workspace in one tab affect another?
# Check if storage event listener exists anywhere else
rg -n "storage.*event\|window.addEventListener.*storage" web-ui/src/ -B 2 -A 2Repository: frankbria/codeframe
Length of output: 45
🏁 Script executed:
# Check the full types to understand SWR dependencies
cat -n web-ui/src/app/prd/page.tsx | sed -n '28,50p'Repository: frankbria/codeframe
Length of output: 866
🏁 Script executed:
# Final verification: check if /prd page has any way to switch workspace inline
cat -n web-ui/src/app/prd/page.tsx | sed -n '50,100p'Repository: frankbria/codeframe
Length of output: 2012
Add workspace change listener to keep PRD page in sync with workspace selection.
When the user switches workspaces (via the home page or another application instance), the workspaceChanged event is dispatched. The /prd page currently reads the workspace path only on mount and never updates it, causing the page to display stale data from the previously selected workspace.
Subscribe to the workspaceChanged event to update the workspace path when it changes:
♻️ Proposed fix
useEffect(() => {
setWorkspacePath(getSelectedWorkspacePath());
setWorkspaceReady(true);
+
+ const onWorkspaceChanged = () => {
+ setWorkspacePath(getSelectedWorkspacePath());
+ };
+ window.addEventListener('workspaceChanged', onWorkspaceChanged);
+ return () => window.removeEventListener('workspaceChanged', onWorkspaceChanged);
}, []);🤖 Prompt for AI Agents
In `@web-ui/src/app/prd/page.tsx` around lines 23 - 26, The PRD page currently
only reads getSelectedWorkspacePath() on mount; update the useEffect to
subscribe to the global "workspaceChanged" event and call
setWorkspacePath(getSelectedWorkspacePath()) (and setWorkspaceReady(true) if
needed) whenever the event fires so the page stays in sync; add a cleanup
function to remove the listener to avoid leaks. Locate the useEffect block in
page.tsx (the one calling setWorkspacePath/getSelectedWorkspacePath and
setWorkspaceReady) and implement the event listener registration and teardown
there.
| // Auto-start when panel mounts if no session yet | ||
| useEffect(() => { | ||
| if (!sessionId) startSession(); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
React StrictMode double-mount will fire startSession twice, potentially creating duplicate backend sessions.
In development with React 18+/19 StrictMode, effects run twice on mount. Since sessionId is still null when the second invocation fires (state updates are batched), startSession() will be called twice, creating two discovery sessions on the backend.
Use an abort/cleanup pattern or a ref guard:
🛡️ Proposed fix using a ref guard
+import { useState, useCallback, useEffect, useRef } from 'react';
...
+ const startedRef = useRef(false);
+
// Auto-start when panel mounts if no session yet
useEffect(() => {
- if (!sessionId) startSession();
+ if (!startedRef.current && !sessionId) {
+ startedRef.current = true;
+ startSession();
+ }
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);🤖 Prompt for AI Agents
In `@web-ui/src/components/prd/DiscoveryPanel.tsx` around lines 67 - 71, The
useEffect in DiscoveryPanel that auto-starts sessions calls startSession() twice
under React StrictMode because sessionId is still falsy during the double-mount;
guard against duplicate backend session creation by adding a ref-based guard or
abort flag: introduce a local mutable ref (e.g., startedRef) checked inside the
useEffect before calling startSession(), set startedRef.current = true
immediately after calling startSession(), and clear it in a cleanup if needed;
ensure the effect still depends on sessionId (or keep empty deps but use the
ref) so startSession is only invoked once even when React mounts twice.
| const resetForm = () => { | ||
| setContent(''); | ||
| setTitle(''); | ||
| setFileName(null); | ||
| setError(null); |
There was a problem hiding this comment.
🟢 Low
prd/UploadPRDModal.tsx:38 Consider clearing fileInputRef.current.value in resetForm. Without this, re-uploading the same file after cancel/error won't trigger onChange since the input value hasn't changed.
| const resetForm = () => { | |
| setContent(''); | |
| setTitle(''); | |
| setFileName(null); | |
| setError(null); | |
| const resetForm = () => { | |
| setContent(''); | |
| setTitle(''); | |
| setFileName(null); | |
| setError(null); | |
| if (fileInputRef.current) fileInputRef.current.value = ''; |
🚀 Want me to fix this? Reply ex: "fix it for me".
1 participant
Summary
docs/PHASE_3_UI_ARCHITECTURE.md/prdroute with markdown editor, AI-powered Socratic discovery panel, PRD upload modal, task generation, and associated task summaryuseEventSource,useTaskStream) for real-time streaming infrastructureNew Components (10)
PRDViewPRDHeaderMarkdownEditorDiscoveryPanelDiscoveryTranscriptDiscoveryInputUploadPRDModalAssociatedTasksSummaryAppSidebarAppLayoutNew Hooks (2)
useEventSourceuseTaskStream/api/v2/tasks/{id}/streamexecution eventsAcceptance Criteria (from #330)
Closes #330
Test plan
npm test— 152 tests pass (64 new)npx tsc --noEmit— zero new type errors/prd, verify empty state rendersSummary by CodeRabbit
New Features
Tests
Documentation
Chores