fix(websocket,discovery): Fix JWT auth and improve discovery UX - #184
Conversation
## WebSocket Authentication - Migrate WebSocket endpoint from session-table auth to JWT tokens - WebSocket now uses same JWT validation as HTTP endpoints - Update tests to mock JWT auth instead of session table ## Discovery Process Improvements - Start discovery automatically when agent starts (was staying idle) - Pass project description as context to generate intelligent first question - Use Claude to generate contextual questions instead of fixed list - Map backend "text" field to frontend "question" field - Add WebSocket broadcast for immediate UI feedback on discovery start - Handle "agent already running" gracefully (info log, not error) ## Server Startup - Load .env file at server startup via load_environment() - Safe for production: load_dotenv() doesn't override existing env vars ## Files Changed - codeframe/ui/routers/websocket.py - JWT auth implementation - codeframe/ui/shared.py - Auto-start discovery, pass description - codeframe/agents/lead_agent.py - AI-driven discovery questions - codeframe/ui/routers/discovery.py - Field name mapping - codeframe/ui/routers/agents.py - WebSocket broadcast - codeframe/ui/server.py - Load .env at startup - web-ui/src/components/DiscoveryProgress.tsx - WebSocket listener - tests/ui/* - Updated for JWT auth Fixes production WebSocket failures and improves discovery UX
WalkthroughAdds AI-driven discovery startup (Claude-generated first question persisted in-memory and DB), background PRD generation with staged WebSocket broadcasts and retry, JWT-based WebSocket authentication with async user lookup, discovery restart/retry endpoints, frontend PRD UI/states and tests, plus persistence and startup tweaks. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Frontend
participant API as Backend
participant Agent as LeadAgent
participant Claude as Claude API
participant DB as Database
participant WS as WebSocket
User->>UI: Trigger Start Discovery (optional description)
UI->>API: POST /agents/start (project_id, description)
API->>Agent: start_discovery(project_description)
Agent->>Claude: Prompt (required topics + context)
alt Claude succeeds
Claude-->>Agent: AI-generated question text
Agent->>DB: Persist discovery state (_current_question_text + _current_question_id)
else Claude fails
Agent->>DB: Persist fallback/framework question
end
Agent-->>API: Return first question text
API->>WS: Broadcast discovery_question_ready
WS-->>UI: Notify frontend
UI->>API: POST /discovery/answer
API->>Agent: Record answer, determine next step
alt discovery complete
Agent->>DB: Mark discovery complete
API->>WS: Broadcast discovery_completed
API->>API: enqueue generate_prd_background()
API->>WS: Broadcast prd_generation_started
loop PRD generation
API->>WS: Broadcast prd_generation_progress (stage, progress_pct, message)
end
API->>DB: Save PRD (key="content")
API->>WS: Broadcast prd_generation_completed (prd_preview)
else not complete
Agent->>DB: Save next question
API->>WS: Broadcast discovery_question_ready
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Code Review SummaryThis PR makes important improvements to WebSocket authentication and discovery UX. The changes are well-structured and address critical security issues. However, there are several areas that need attention before merging. ✅ Strengths
🔴 Critical Issues1. Duplicate State Persistence (codeframe/agents/lead_agent.py:366-371)You're saving current_question_text twice in the same function:
Issue: This creates inconsistency. If _save_discovery_state() doesn't save _current_question_text, then the second save is needed but the method should be updated. If it does save it, this is redundant. Recommendation: Update _save_discovery_state() to include current_question_text and remove the duplicate save. 2. Missing Error Recovery (codeframe/ui/shared.py:310-317)When start_discovery() fails, you log the error and continue, but you don't update the project phase or broadcast any failure notification. Issue: Frontend will show 'starting discovery' indefinitely if this fails. Recommendation: Add error state handling with a WebSocket broadcast of type 'discovery_error'. 3. Token Security Logging (codeframe/auth/dependencies.py:36)You're logging whether credentials are present. While helpful for debugging, this could leak information in production logs. Recommendation: Remove this debug log or move it behind a debug flag (logger.isEnabledFor(logging.DEBUG)).
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ui/conftest.py (1)
82-105: Removecreate_test_session_tokenfunction as unused code.This function is never called anywhere in the codebase and can be safely deleted.
🧹 Nitpick comments (8)
web-ui/src/components/DiscoveryProgress.tsx (2)
102-114: Consider potential race condition withsetIsStarting(false)placement.Moving
setIsStarting(false)intofetchProgress's finally block means it will be cleared on every fetch, including the 500ms delayed fetch triggered bydiscovery_startingWebSocket message. If the discovery hasn't actually started yet (backend still processing), the button state may briefly flicker.This is a minor UX concern since the state will stabilize once the actual data arrives, but worth noting.
🔎 Alternative: clear isStarting only when discovery state changes
const fetchProgress = useCallback(async () => { try { const response = await projectsApi.getDiscoveryProgress(projectId); setData(response.data); setError(null); + // Clear isStarting only if discovery has progressed past idle + if (response.data?.discovery?.state !== 'idle') { + setIsStarting(false); + } } catch (err) { setError('Failed to load discovery progress'); console.error('Error fetching discovery progress:', err); + setIsStarting(false); } finally { setLoading(false); - setIsStarting(false); } }, [projectId]);
177-190: Consider addingfetchProgressto the dependency array.Now that
fetchProgressis wrapped inuseCallback, it should be safe to include in the dependency array, and the eslint-disable comment can be removed.🔎 Proposed fix
return () => clearInterval(intervalId); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [data]); + }, [data, fetchProgress]);codeframe/ui/routers/websocket.py (2)
56-59: Update docstring to reflect JWT authentication.The docstring mentions "sessions table" validation, but the implementation now uses JWT token validation.
🔎 Proposed fix
Authentication: - Requires token as query parameter: ws://host/ws?token=YOUR_SESSION_TOKEN - - Token is validated against sessions table on connection + - JWT token is validated on connection (same as HTTP endpoints) - User ID is extracted and stored with WebSocket connection - Project access is checked on subscribe/unsubscribe messages
102-141: JWT validation duplicates logic fromauth/dependencies.py.The JWT decode and user verification logic (lines 102-141) is nearly identical to the code in
auth/dependencies.py(lines 62-111). Consider extracting this to a shared helper to maintain consistency and reduce duplication.This is acceptable for now since WebSocket authentication has different error handling (WebSocket close vs HTTP exception), but worth tracking for future refactoring.
Would you like me to generate a shared helper function that can be used by both HTTP and WebSocket authentication?
tests/ui/conftest.py (1)
27-47: Consider consolidating duplicatecreate_test_jwt_tokenimplementations.This function is defined identically in at least 4 locations:
tests/ui/conftest.py(this file)tests/api/conftest.pytests/helpers/__init__.pytests/conftest.pyConsider importing from a single source (e.g.,
tests/helpers/__init__.pyortests/conftest.py) to reduce maintenance burden.codeframe/ui/routers/agents.py (2)
100-109: Consider wrapping broadcast in try/except for resilience.If
manager.broadcast()raises an exception, the endpoint will fail beforebackground_tasks.add_task()is called, preventing discovery from starting. Other broadcast calls incodeframe/ui/shared.py(lines 325-337, 340-346) wrap broadcasts in try/except to ensure startup continues even if broadcast fails.🔎 Proposed fix
# Broadcast immediate feedback before background task starts + try: await manager.broadcast( { "type": "discovery_starting", "project_id": project_id, "status": "starting", "timestamp": time.time(), }, project_id=project_id ) + except Exception: + # Continue even if broadcast fails + pass
142-151: Same concern: wrap broadcast in try/except for resilience.Similar to the previous broadcast, this one should also be wrapped to prevent endpoint failure if broadcasting encounters an issue.
🔎 Proposed fix
# Broadcast immediate feedback before background task starts + try: await manager.broadcast( { "type": "discovery_starting", "project_id": project_id, "status": "starting", "timestamp": time.time(), }, project_id=project_id ) + except Exception: + # Continue even if broadcast fails + passcodeframe/agents/lead_agent.py (1)
293-316:_save_discovery_statedoesn't persist_current_question_text.While
start_discoverymanually savescurrent_question_textto the database (lines 365-371),_save_discovery_stateonly savesstateandcurrent_question_id. This creates an inconsistency where calling_save_discovery_stateelsewhere might not persist the question text.Consider adding the question text to
_save_discovery_statefor consistency:🔎 Proposed fix
def _save_discovery_state(self) -> None: """Save discovery state to database.""" try: # Save current state self.db.create_memory( project_id=self.project_id, category="discovery_state", key="state", value=self._discovery_state, ) # Save current question ID if exists if self._current_question_id: self.db.create_memory( project_id=self.project_id, category="discovery_state", key="current_question_id", value=self._current_question_id, ) + # Save current question text if exists (for AI-generated questions) + if self._current_question_text: + self.db.create_memory( + project_id=self.project_id, + category="discovery_state", + key="current_question_text", + value=self._current_question_text, + ) logger.debug(f"Saved discovery state: {self._discovery_state}") except Exception as e: logger.error(f"Failed to save discovery state: {e}")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
codeframe/agents/lead_agent.pycodeframe/auth/dependencies.pycodeframe/ui/routers/agents.pycodeframe/ui/routers/discovery.pycodeframe/ui/routers/websocket.pycodeframe/ui/server.pycodeframe/ui/shared.pytests/ui/conftest.pytests/ui/test_websocket_router.pyweb-ui/package.jsonweb-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/types/index.tsweb-ui/src/components/DiscoveryProgress.tsx
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/shared.pycodeframe/ui/server.pycodeframe/ui/routers/discovery.pycodeframe/agents/lead_agent.pycodeframe/ui/routers/websocket.pycodeframe/auth/dependencies.pycodeframe/ui/routers/agents.py
codeframe/auth/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/auth/**/*.py: For authentication, use FastAPI Users with JWT tokens and mandatory authentication (no bypass mode)
Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Files:
codeframe/auth/dependencies.py
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Implement Lead Agent for orchestration and Worker Agents for specialization (Backend, Frontend, Test, Review) with maturity levels D1-D4
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/types/index.tsweb-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Applied to files:
web-ui/src/types/index.tstests/ui/conftest.pycodeframe/ui/routers/websocket.pytests/ui/test_websocket_router.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/types/index.tscodeframe/ui/shared.pycodeframe/ui/routers/agents.pyweb-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Applied to files:
web-ui/src/types/index.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : For authentication, use FastAPI Users with JWT tokens and mandatory authentication (no bypass mode)
Applied to files:
tests/ui/conftest.pycodeframe/ui/routers/websocket.pytests/ui/test_websocket_router.py
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/codeframe/ui/**/*.py : Use FastAPI with Uvicorn for the async API backend and WebSockets for real-time communication
Applied to files:
codeframe/ui/routers/websocket.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/ui/routers/websocket.pycodeframe/auth/dependencies.pycodeframe/ui/routers/agents.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/package.jsonweb-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/tests/**/*.py : Use pytest fixtures for Python testing and avoid over-mocking
Applied to files:
tests/ui/test_websocket_router.py
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use functional React components with TypeScript interfaces
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
🧬 Code graph analysis (7)
tests/ui/conftest.py (3)
tests/api/conftest.py (1)
create_test_jwt_token(43-63)tests/conftest.py (1)
create_test_jwt_token(12-35)tests/helpers/__init__.py (1)
create_test_jwt_token(11-31)
codeframe/ui/shared.py (2)
codeframe/persistence/database.py (2)
get_project(278-280)update_project(286-288)codeframe/agents/lead_agent.py (1)
start_discovery(318-384)
codeframe/ui/server.py (1)
codeframe/core/config.py (1)
load_environment(194-207)
codeframe/agents/lead_agent.py (3)
codeframe/persistence/database.py (1)
create_memory(502-504)codeframe/discovery/questions.py (2)
generate_questions(61-171)get_next_question(173-207)codeframe/core/project.py (1)
chat(689-700)
codeframe/ui/routers/websocket.py (3)
codeframe/ui/dependencies.py (1)
get_db_websocket(50-65)codeframe/persistence/database.py (1)
Database(51-698)codeframe/auth/manager.py (1)
get_async_session_maker(104-113)
codeframe/ui/routers/agents.py (1)
codeframe/ui/shared.py (2)
start_agent(257-381)broadcast(154-185)
web-ui/src/components/DiscoveryProgress.tsx (2)
web-ui/src/lib/websocket.ts (1)
getWebSocketClient(193-198)web-ui/src/types/index.ts (1)
WebSocketMessage(110-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (18)
codeframe/auth/dependencies.py (1)
36-37: LGTM!The debug logging provides useful visibility into authentication flow without exposing sensitive data (only logging presence as a boolean).
web-ui/src/types/index.ts (1)
103-103: LGTM!The new
discovery_startingmessage type follows the established pattern and enables real-time UI feedback. The inline comment clearly documents its purpose.web-ui/src/components/DiscoveryProgress.tsx (3)
8-14: LGTM!The new imports are properly organized and support the WebSocket integration for real-time discovery feedback.
125-136: LGTM!The fallback polling pattern with a 2-second delay ensures reliability when WebSocket messages are delayed or missed. The comment clearly documents the intended flow.
143-175: LGTM!The WebSocket listener implementation is well-structured:
- Properly filters messages by project_id
- Handles relevant event types for discovery progress updates
- Correctly cleans up the subscription on unmount
- Dependencies are appropriate for the effect
codeframe/ui/routers/websocket.py (2)
12-25: LGTM!The imports are correctly organized to support the JWT-based authentication flow, importing from the centralized auth module.
35-46: LGTM!The health check endpoint is useful for monitoring and E2E tests to verify WebSocket server availability before attempting connections.
codeframe/ui/server.py (1)
154-157: LGTM!Loading environment variables at the start of lifespan ensures
AUTH_SECRET,ANTHROPIC_API_KEY, and other configuration values are available before security validation. The inline import avoids potential circular dependency issues.codeframe/ui/routers/discovery.py (1)
237-246: LGTM!The field mapping correctly transforms the backend's
textfield to the frontend's expectedquestionfield. The explicitNonehandling and use of.get()with defaults ensures robustness against missing data.tests/ui/conftest.py (1)
146-148: LGTM! JWT migration is correct.The switch from
create_test_session_token(db)tocreate_test_jwt_token()aligns with the WebSocket authentication migration to JWT tokens.codeframe/ui/routers/agents.py (1)
11-11: LGTM! Imports are correctly added.The
timeandmanagerimports are necessary for the new broadcast functionality.Also applies to: 22-22
codeframe/ui/shared.py (2)
276-285: LGTM! Idempotent behavior is the right pattern.Changing from
ValueErrorto logging at INFO level with early return provides better resilience for concurrent start requests and race conditions. This is consistent with the documented idempotent behavior.
301-322: LGTM! Discovery auto-start implementation is well-structured.The flow correctly:
- Fetches project description via
asyncio.to_threadfor async compatibility- Handles missing description gracefully with empty string default
- Wraps
start_discoverycall in try/except to ensure agent startup continues even if discovery fails- Updates project status and phase atomically
tests/ui/test_websocket_router.py (2)
29-31: LGTM! Mock WebSocket correctly uses JWT token.The change from a session token to
"test-jwt-token"aligns with the JWT authentication migration. Based on learnings, WebSocket connections use authentication tokens passed as query parameters (?token=TOKEN).
56-98: LGTM! JWT authentication test scaffolding is well-implemented.The fixtures correctly:
- Mock
pyjwt.decodeto return a valid JWT payload with proper claims (sub,aud)- Mock
get_async_session_makerwith an async context manager that yields a mock session- Use
autouse=Trueto ensure all WebSocket tests automatically get JWT auth patched- Provide a mock user with
id=1andis_active=TrueThe patch targets match the actual imports and usage in
codeframe/ui/routers/websocket.py(line 12:import jwt as pyjwt, line 23:get_async_session_maker). The async context manager pattern for session mocking is appropriate for SQLAlchemy async sessions, and the test setup correctly simulates the mandatory authentication flow with both JWT validation and user verification.codeframe/agents/lead_agent.py (3)
318-384: LGTM! AI-driven discovery initialization is well-implemented.The implementation:
- Correctly transitions state to "discovering" before API call
- Stores project description as context for future reference
- Uses Claude to generate contextual first question
- Has appropriate fallback to framework questions on error
- Logs at appropriate levels (info for success, error for failures)
The use of
self.chat()ensures the AI question is persisted in conversation history, which is good for context continuity.
386-425: LGTM! Prompt construction is clear and focused.The prompt:
- Clearly defines the AI's role and goals
- Provides project context when available
- Requests ONE focused question to avoid overwhelming users
- Instructs to avoid redundancy with provided description
519-531: LGTM! Status reporting correctly handles AI-generated questions.The special handling for
_current_question_id == "ai_question"ensures the UI receives the actual AI-generated text rather than attempting a failed framework lookup. The structure matches the expected format withid,category,text, andimportancefields.
- Show spinner in submit button while submitting answer - Show "Generating next question..." with spinner after submission - Hide question and input form while loading next question - Remove 1-second delay - fetch immediately after submission Improves UX with immediate visual feedback during answer submission
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
web-ui/src/components/DiscoveryProgress.tsx (5)
175-188: Consider addingfetchProgressto the dependency array.Since
fetchProgressis now a stableuseCallback, it can safely be included in the dependency array. This would allow removing the eslint-disable comment and ensure the interval uses the current callback ifprojectIdchanges mid-discovery.🔎 Proposed fix
return () => clearInterval(intervalId); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [data]); + }, [data, fetchProgress]);
238-249: Replace inline SVG spinners with Hugeicons.As per coding guidelines, all icon usage should use Hugeicons (
@hugeicons/react) instead of inline SVGs. Consider using a loading/spinner icon from the Hugeicons library for consistency.Based on learnings, Hugeicons should be used for all icons in
web-ui/src/**/*.tsx.Also applies to: 301-306
333-340: Replace hardcoded green colors with semantic palette.The completed state uses hardcoded Tailwind colors (
bg-green-50,border-green-200,text-green-600,text-green-800). Per coding guidelines, use shadcn/ui Nova semantic color palette instead.🔎 Proposed fix using semantic colors
{isCompleted && ( - <div className="flex items-center gap-2 p-4 bg-green-50 rounded-lg border border-green-200"> - <span className="text-green-600 text-lg">✓</span> - <span className="text-sm font-medium text-green-800"> + <div className="flex items-center gap-2 p-4 bg-primary/10 rounded-lg border border-primary"> + <span className="text-primary text-lg">✓</span> + <span className="text-sm font-medium text-foreground"> Discovery Complete </span> </div> )}As per coding guidelines, semantic color palette should be used.
123-133: Minor: Fallback timeout could be cleared if WebSocket responds first.The 2-second fallback setTimeout will still fire even if the WebSocket
discovery_startingoragent_startedmessage already triggered a refresh. This causes an unnecessary duplicate API call. Consider storing the timeout ID and clearing it when WebSocket-triggeredfetchProgresscompletes.
141-173: Debounce multiple rapidfetchProgresscalls triggered by WebSocket messages.The code triggers
fetchProgress()directly onagent_started(line 160) andstatus_update(line 165) without delay. If these messages arrive in quick succession, this causes multiple simultaneous API calls. Consider debouncing or coalescing these refreshes (e.g., using a timer flag) to reduce redundant network requests.Note:
'discovery_starting','agent_started', and'status_update'are all properly defined in theWebSocketMessageTypeunion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
web-ui/src/components/DiscoveryProgress.tsx
🧰 Additional context used
📓 Path-based instructions (3)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
🧬 Code graph analysis (1)
web-ui/src/components/DiscoveryProgress.tsx (2)
web-ui/src/lib/websocket.ts (1)
getWebSocketClient(193-198)web-ui/src/types/index.ts (1)
WebSocketMessage(110-173)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (3)
web-ui/src/components/DiscoveryProgress.tsx (3)
8-14: LGTM!The new imports for
useCallback,getWebSocketClient, andWebSocketMessageare appropriate for the WebSocket integration and memoization changes.
66-87: LGTM!The submit flow correctly sequences state transitions: sets loading state before fetching, clears the answer immediately on success, and properly resets
isLoadingNextQuestionin both success and error paths. Good UX improvement by showing immediate feedback.
100-112: LGTM!Wrapping
fetchProgressinuseCallbackwithprojectIddependency ensures a stable reference for effect dependencies. ClearingisStartingin thefinallyblock is a clean approach that handles both success and error cases.
- Add discovery_completed WebSocket message type - Show "Discovery Complete — All questions answered" banner - Display PRD generation status with spinner when generating - Show "Ready for PRD Generation" when completed but not yet generating - Set isGeneratingPRD when last answer is submitted Improves UX by showing clear next steps after discovery phase
Updated Code Review - New CommitsI've reviewed the 2 additional commits since my last review (3af2258 and fa630c9). Here's my assessment: ✅ Improvements in New CommitsCommit 2: Loading Spinner (3af2258)Good improvements:
Remaining issue:
Fix needed: // In submitAnswer() - line 70-76
try {
const response = await projectsApi.getDiscoveryProgress(projectId);
setData(response.data);
// ... rest of logic
} catch (error) {
// error handling
} finally {
setIsLoadingNextQuestion(false); // Move here to ensure cleanup
}Commit 3: PRD Generation Status (fa630c9)Good additions:
New issue identified:
Fix needed: useEffect(() => {
// Clear PRD generation spinner when PRD exists in data
if (data?.discovery?.prd && isGeneratingPRD) {
setIsGeneratingPRD(false);
}
}, [data, isGeneratingPRD]);📊 Status of Previous Critical IssuesFrom my original review, here's what's been addressed: ❌ Still Outstanding:
✅ Addressed by new commits:
🎯 Updated RecommendationBefore merging, must fix:
Should fix (but not blocking):
The PR is getting close! The UX improvements are solid, but the error handling gaps need to be closed before production deployment. Test plan gaps remain:
Would you like me to create a follow-up issue for the test coverage, or would you prefer to add tests in this PR? |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
web-ui/src/components/DiscoveryProgress.tsx (3)
150-188: WebSocket integration looks solid.The event handlers correctly filter by
project_idand trigger progress refreshes for relevant discovery lifecycle events. The dependency array is correct with bothprojectIdand the memoizedfetchProgress.Optional: Consider debouncing rapid
fetchProgresscalls.If multiple WebSocket events arrive in quick succession (e.g.,
agent_startedfollowed bystatus_update),fetchProgresswill be called multiple times. While the API call is idempotent, debouncing could reduce unnecessary network traffic:🔎 Example debounce pattern
// Add at the top of the component const fetchProgressDebounced = useMemo( () => debounce(fetchProgress, 300), [fetchProgress] ); // Use in handlers if (message.type === 'status_update') { fetchProgressDebounced(); }
191-203: Auto-refresh effect has disabled dependency warning.The effect uses
fetchProgressbut excludes it from the dependency array (Line 203). WhilefetchProgressis stable due touseCallback, disabling the lint rule can mask potential stale closure issues if the callback logic changes.🔎 Recommended fix
Include
fetchProgressin the dependency array:return () => clearInterval(intervalId); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [data]); + }, [data, fetchProgress]);Since
fetchProgressis memoized with[projectId], this won't cause excessive re-renders.
253-264: Good loading state feedback, but spinner code is duplicated.The loading state provides clear feedback while generating the next question. However, the SVG spinner markup (Lines 257-260) is duplicated in Lines 317-320 and 363-366.
🔎 Extract spinner into a reusable component
Create a
Spinner.tsxcomponent:interface SpinnerProps { size?: 'sm' | 'md' | 'lg'; className?: string; } export const Spinner = ({ size = 'md', className = '' }: SpinnerProps) => { const sizeClasses = { sm: 'h-4 w-4', md: 'h-5 w-5', lg: 'h-6 w-6' }; return ( <svg className={`animate-spin ${sizeClasses[size]} ${className}`} xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" > <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="4"></circle> <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z"></path> </svg> ); };Then replace all three instances with
<Spinner className="text-primary" />.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/types/index.ts
🧰 Additional context used
📓 Path-based instructions (3)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (4)
web-ui/src/components/DiscoveryProgress.tsx (4)
8-13: LGTM! Clean imports for WebSocket integration.The added imports are properly typed and necessary for the WebSocket-driven discovery updates.
69-83: Well-structured submission flow with proper state coordination.The immediate transition to loading state (Line 71) provides instant feedback, and the subsequent progress fetch ensures the UI stays synchronized with the backend. Error handling at Line 95 properly clears loading states.
109-121: LGTM! Proper use ofuseCallbackfor stability.Wrapping
fetchProgressinuseCallbackwith theprojectIddependency ensures a stable reference for the WebSocket effect (Line 188) and prevents unnecessary re-subscriptions.
310-324: Submit button provides clear visual feedback.The button correctly shows a spinner and disabled state during submission, providing good UX feedback. The spinner duplication is already noted in the previous comment.
Backend: - Add generate_prd_background() task in discovery router - Trigger PRD generation automatically when last answer submitted - Broadcast WebSocket events: prd_generation_started, completed, failed - Update project phase to "planning" after PRD generation Frontend: - Add prd_generation_* WebSocket message types - Handle PRD generation states (generating, completed, failed) - Show spinner during PRD generation - Show success message when PRD is ready - Show error message if PRD generation fails Provides visual feedback throughout the entire discovery → PRD flow
Updated Code Review - Latest CommitsI've reviewed the latest changes (commits 3af2258, fa630c9, and 00e4587). Here's my updated assessment: ✅ Recent ImprovementsThe latest commits address several UX issues:
🔴 Critical Issues Remaining1. Missing finally block in submitAnswer() (DiscoveryProgress.tsx:71-80)Current code: setIsSubmitting(false);
setIsLoadingNextQuestion(true);
// ... fetch progress ...
setIsLoadingNextQuestion(false);Problem: If Fix needed: try {
setIsSubmitting(false);
setIsLoadingNextQuestion(true);
const response = await projectsApi.getDiscoveryProgress(projectId);
setData(response.data);
if (response.data.discovery?.state === 'completed') {
setIsGeneratingPRD(true);
}
} finally {
setIsLoadingNextQuestion(false);
}2. Duplicate state persistence (codeframe/agents/lead_agent.py:362-371)You're saving
Issue: This creates potential inconsistency. Either Recommendation: Update 3. Missing error recovery (codeframe/ui/shared.py:310-317)When
Issue: Frontend shows 'starting discovery' indefinitely if this fails. Fix needed: Add WebSocket broadcast of type
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web-ui/src/components/DiscoveryProgress.tsx (2)
373-378: Hardcoded green colors still present.This banner continues to use hardcoded Tailwind color classes (
bg-green-50,border-green-200,text-green-600,text-green-800) which violates the coding guidelines requiring shadcn/ui Nova's semantic color palette. This was flagged in a previous review.As per coding guidelines, use semantic colors like
bg-card,text-foreground,border, etc.Based on coding guidelines, all color values should use the semantic palette from shadcn/ui Nova.
374-374: Emoji icons should be replaced with Hugeicons.Lines 374, 402, and 410 use emoji characters ("✓", "✗") which should be replaced with Hugeicons as per the coding guidelines. This was flagged in a previous review.
As per coding guidelines, replace all icon usage with Hugeicons (@hugeicons/react).
Based on coding guidelines, use Hugeicons for all icons and avoid emoji characters.
Also applies to: 402-402, 410-410
🧹 Nitpick comments (3)
codeframe/ui/routers/discovery.py (1)
62-62: Consider safer string truncation for preview.The slice
prd_content[:200]could truncate a multi-byte UTF-8 character mid-sequence, potentially causing encoding issues in the WebSocket payload. While this is just a preview and likely non-critical, consider using a helper that respects character boundaries or truncates at word boundaries.🔎 Optional improvement for safer truncation
- "prd_preview": prd_content[:200] if prd_content else "", + "prd_preview": (prd_content[:200] + "..." if len(prd_content) > 200 else prd_content) if prd_content else "",Or for a more robust solution, add a helper function:
def safe_truncate(text: str, max_length: int = 200) -> str: """Safely truncate text at word boundary.""" if not text or len(text) <= max_length: return text or "" return text[:max_length].rsplit(' ', 1)[0] + "..."web-ui/src/components/DiscoveryProgress.tsx (2)
82-85: Potential redundant state update.Lines 82-85 set
isGeneratingPRDwhen discovery completes. However, the WebSocket handler on Line 181 also sets this state when it receives thediscovery_completedevent. While this doesn't cause functional issues (setting the same value twice is idempotent), it may represent a minor redundancy.Consider whether the inline check is needed or if you can rely solely on the WebSocket event for consistency.
275-286: Loading state provides good UX, but consider extracting the spinner.The loading next question block provides clear feedback during the question generation phase. However, the inline SVG spinner (Lines 279-282) is duplicated in multiple places (Lines 339-342, 391-394, 418-421). Consider extracting it to a reusable
Spinnercomponent to reduce duplication and improve maintainability.🔎 Example Spinner component
Create
web-ui/src/components/Spinner.tsx:interface SpinnerProps { size?: number; className?: string; } export function Spinner({ size = 20, className = "text-primary" }: SpinnerProps) { return ( <svg className={`animate-spin ${className}`} width={size} height={size} xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24" > <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="4" /> <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z" /> </svg> ); }Then replace inline spinners with
<Spinner />.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codeframe/ui/routers/discovery.pyweb-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/discovery.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxcodeframe/ui/routers/discovery.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Applied to files:
web-ui/src/types/index.ts
🧬 Code graph analysis (1)
web-ui/src/components/DiscoveryProgress.tsx (3)
web-ui/src/lib/api.ts (1)
projectsApi(30-54)web-ui/src/lib/websocket.ts (1)
getWebSocketClient(193-198)web-ui/src/types/index.ts (1)
WebSocketMessage(114-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (5)
codeframe/ui/routers/discovery.py (2)
86-93: Background task integration looks good.The addition of
BackgroundTasksand the conditional trigger for PRD generation after discovery completion follows FastAPI best practices. The PRD generation will run asynchronously after the response is sent, providing a good UX.Also applies to: 201-204
301-310: Field mapping improves frontend compatibility.The mapping of backend
"text"to frontend"question"creates a clearer API contract and prevents blank questions from appearing in the UI. The None handling ensures robustness when no current question exists.web-ui/src/types/index.ts (1)
103-112: Type definitions align well with backend changes.The new WebSocket message types (
discovery_starting,discovery_completed,prd_generation_*) and corresponding payload fields (total_answers,next_phase) match the events broadcast from the backend indiscovery.py. The inline comments provide helpful context.Also applies to: 178-180
web-ui/src/components/DiscoveryProgress.tsx (2)
8-8: State and imports appropriately extended for PRD workflow.The addition of
useCallback, WebSocket integration, and PRD-related state variables (isGeneratingPRD,prdCompleted,prdError,isLoadingNextQuestion) properly supports the new real-time feedback and PRD generation features.Also applies to: 11-11, 13-13, 31-31, 38-41
381-429: PRD status section provides comprehensive feedback.The PRD generation status section correctly handles all states (generating, completed, failed, starting) with clear messaging. The logic properly branches based on
isGeneratingPRD,prdCompleted, andprdErrorstate flags.Note: The past review comment about
isGeneratingPRDnever being reset tofalsehas been resolved. Line 193 in the WebSocket handler correctly sets it tofalsewhenprd_generation_completedis received.
- Backend broadcasts progress at distinct stages (gathering_data, calling_llm, saving) - Added prd_generation_progress WebSocket message type with stage, message, progress_pct - Frontend displays current stage with progress bar during PRD generation - User sees descriptive messages like "Gathering discovery answers...", "Generating PRD with AI..." This provides immediate visual feedback during the PRD generation process instead of just showing "Generating..." with no progress indication.
Updated Code Review - Latest CommitsI've reviewed the latest changes in this PR (commits through 3e0ad4a). This builds on previous reviews and focuses on the current state of the code. OverviewThis PR successfully migrates WebSocket authentication from session-based to JWT tokens and adds intelligent AI-driven discovery with real-time PRD generation tracking. The implementation is solid overall with good error handling and UX improvements. What's Working Well
Critical Issues to Address1. Duplicate State Persistence (codeframe/agents/lead_agent.py:362-371) 2. Missing Error Recovery (codeframe/ui/shared.py:310-317) 3. Debug Logging Security (codeframe/auth/dependencies.py:36) Major Issues4. Race Condition in handleStartDiscovery - The 2-second timeout can fire after WebSocket updates arrive. Use a ref to cancel the timeout. 5. Field Mapping in Router - Mapping text to question in the router is fragile. Create a Pydantic response schema. 6. Unused Package Dependency - baseline-browser-mapping added to both dependencies and devDependencies. Why is it needed as production dependency? Minor Issues
Testing Gaps
RecommendationAddress the 3 critical issues (#1-3), then this is ready to merge. Issues #4-8, testing gaps, and documentation updates can be follow-up tasks. Great work overall! Review performed on commits through 3e0ad4a |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web-ui/src/components/DiscoveryProgress.tsx (2)
396-401: Replace hardcoded green colors with semantic palette.The discovery complete banner uses hardcoded Tailwind green classes (
bg-green-50,border-green-200,text-green-600,text-green-800), which violates coding guidelines requiring shadcn/ui Nova's semantic color palette.As per coding guidelines, use semantic colors like
bg-card,text-foreground,border, etc.🔎 Recommended fix using semantic colors
- <div className="flex items-center gap-2 p-4 bg-green-50 rounded-lg border border-green-200"> - <span className="text-green-600 text-lg">✓</span> + <div className="flex items-center gap-2 p-4 bg-success/10 rounded-lg border border-success"> + <span className="text-success text-lg">✓</span> <span className="text-sm font-medium text-green-800"> Discovery Complete — All questions answered </span> </div>If
successis not in your palette, usebg-primary/10andborder-primaryinstead, and ensure the text also uses a semantic class liketext-foregroundortext-primary.Based on coding guidelines.
Also applies to: Lines 404-409, 442, 444-445 use similar hardcoded green colors.
397-397: Replace emoji icons with Hugeicons.Lines 397, 442, and 450 use emoji characters ("✓" and "✗") which should be replaced with Hugeicons as per coding guidelines.
As per coding guidelines, replace all icon usage with Hugeicons (@hugeicons/react).
🔎 Recommended fix using Hugeicons
Import the appropriate icons at the top of the file:
import { Tick01Icon, Cancel01Icon } from '@hugeicons/react';Replace line 397:
- <span className="text-green-600 text-lg">✓</span> + <Tick01Icon className="text-success flex-shrink-0" size={20} />Replace line 442:
- <span className="text-green-600 text-lg flex-shrink-0">✓</span> + <Tick01Icon className="text-success flex-shrink-0" size={20} />Replace line 450:
- <span className="text-destructive text-lg flex-shrink-0">✗</span> + <Cancel01Icon className="text-destructive flex-shrink-0" size={20} />Based on coding guidelines.
Also applies to: 442-442, 450-450
🧹 Nitpick comments (2)
codeframe/ui/routers/discovery.py (1)
42-42: Consider moving the asyncio import to module level.Importing
asyncioinside the function works but deviates from convention. Module-level imports improve readability and make dependencies explicit.🔎 Proposed refactor
Move the import to the top of the file with other imports:
import os import logging +import asyncio from typing import Dict, AnyThen remove line 42:
- import asyncio - async def broadcast_progress(stage: str, message: str, progress_pct: int = 0):web-ui/src/components/DiscoveryProgress.tsx (1)
216-226: Optimize error field access order for prd_generation_failed.Based on past review feedback, the backend sends the error at the root level (
message.error), not inmessage.data.error. While the current code has a fallback, it triesmessage.data?.errorfirst, which will always be undefined, causing an unnecessary check.🔎 Recommended optimization
if (message.type === 'prd_generation_failed') { setIsGeneratingPRD(false); setPrdCompleted(false); // Extract error from data or use default message - const errorMsg = message.data?.error || - (message as { error?: string }).error || + const errorMsg = (message as { error?: string }).error || + message.data?.error || 'PRD generation failed'; setPrdError(errorMsg); setPrdStage('failed'); setPrdProgressPct(0); }Based on learnings from past review.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codeframe/ui/routers/discovery.pyweb-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
🧰 Additional context used
📓 Path-based instructions (4)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/discovery.py
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use PascalCase for React component names
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Applied to files:
web-ui/src/types/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (10)
codeframe/ui/routers/discovery.py (3)
11-11: LGTM!The
BackgroundTasksimport is necessary for the new PRD generation background task functionality and follows standard FastAPI patterns.
129-129: LGTM! Background task integration is correct.The addition of
BackgroundTasksparameter and the triggering of PRD generation after discovery completion follows the correct pattern. The ordering ensures:
- Discovery completion is broadcast immediately for UI feedback
- Long-running PRD generation happens asynchronously without blocking the response
- The user receives confirmation before the expensive LLM operation begins
This aligns with the PR objective of improving UX through immediate feedback.
Also applies to: 240-243
340-349: LGTM! Field mapping correctly aligns backend and frontend contracts.The transformation from backend
"text"to frontend"question"prevents blank questions in the UI and establishes a clear API contract. The defensive programming with.get()and explicit None handling ensures robustness.web-ui/src/types/index.ts (2)
103-113: LGTM - WebSocket message types properly extended.The new message types for discovery and PRD generation lifecycle events are well-documented and properly typed. The comments clearly explain the purpose of each event, which aligns with the PR's goal of improving discovery UX with real-time feedback.
179-187: LGTM - PRD and discovery fields properly typed.The new optional fields are correctly typed and well-documented. The inline comments clearly indicate which WebSocket events use each field, making the interface easy to understand and maintain.
web-ui/src/components/DiscoveryProgress.tsx (5)
8-8: LGTM - Imports and state properly set up for WebSocket integration.The new imports and state variables are well-structured to support real-time PRD generation progress tracking. Using
useCallbackfor memoization is a good performance optimization.Also applies to: 11-13, 31-31, 39-44
74-88: LGTM - Improved answer submission flow with immediate feedback.The removal of the artificial delay and immediate next-question fetch provides better UX. The state transitions are properly managed, and the PRD generation trigger when discovery completes is correctly implemented.
114-126: LGTM - fetchProgress properly memoized with useCallback.Wrapping in
useCallbackis the right approach for a function used in effect dependencies. Thefinallyblock correctly resets the starting state regardless of success or failure.
137-148: LGTM - Resilient discovery start with WebSocket + fallback.The combination of WebSocket-driven updates with a setTimeout fallback provides good resilience. The 2-second fallback delay is reasonable for cases where WebSocket delivery is slow.
299-309: LGTM - Loading states provide clear visual feedback.The conditional rendering of loading states, current question, and input fields provides excellent UX. Users get immediate feedback when submitting answers and waiting for the next question.
Also applies to: 312-312, 324-324, 361-366
The discovery progress endpoint was failing with "Error retrieving
discovery progress percentage" because it tried to access keys like
progress_percentage, total_required, remaining_count directly from the
status dict. These keys may not exist if discovery state is "discovering"
but current_question_id is None (edge case/race condition).
Changed from status["key"] to status.get("key", default) for all
potentially missing keys.
Code Review - PR #184I've reviewed the changes in this PR focusing on code quality, security, performance, and test coverage. Overall, this is a solid improvement that addresses critical technical debt with the WebSocket authentication migration and adds valuable UX enhancements to the discovery process. ✅ Strengths
🔴 Critical Issues1. Duplicate State Persistence (
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
codeframe/ui/routers/discovery.py (1)
76-84: Add timeout protection for the LLM API call.The call to
agent.generate_prd()on line 84 lacks timeout protection. If the Claude API experiences delays or hangs, this background task could run indefinitely.This concern has already been flagged in a previous review. Please refer to the existing comment with the proposed fix using
asyncio.wait_for()with a 300-second timeout.
🧹 Nitpick comments (1)
codeframe/ui/routers/discovery.py (1)
95-122: Consider handling partial state on PRD generation failure.If PRD generation fails after discovery completes, the project remains in "discovery" phase while the discovery state is "completed". This could create a confusing UX where discovery is done but the phase hasn't advanced.
Consider either:
- Updating the project phase to "planning" before calling the LLM (optimistic update, rollback on failure)
- Adding a transitional phase like "prd_generation" to reflect the current state more accurately
- Documenting that users should retry from the UI if PRD generation fails
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codeframe/ui/routers/discovery.py
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/discovery.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/codeframe/**/*.py : Use async/await for I/O operations such as database and API calls in Python
Applied to files:
codeframe/ui/routers/discovery.py
🧬 Code graph analysis (1)
codeframe/ui/routers/discovery.py (1)
codeframe/ui/shared.py (1)
broadcast(154-185)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (3)
codeframe/ui/routers/discovery.py (3)
11-11: LGTM! BackgroundTasks import is appropriate.The addition of
BackgroundTasksfrom FastAPI is correctly used to enable non-blocking PRD generation after discovery completion.
129-129: LGTM! Background task integration is correct.The
BackgroundTasksparameter is properly added and the PRD generation task is triggered appropriately upon discovery completion with the correct arguments.Also applies to: 241-243
330-350: LGTM! Excellent defensive coding and field mapping.The safe dictionary access with
.get()and default values prevents KeyErrors in edge cases (e.g., race conditions where discovery state is "discovering" but keys are missing). The mapping of backend "text" to frontend "question" field provides clear API semantics.
Backend: - Always calculate progress fields when state is "discovering", even if current_question_id is None - This fixes edge case where discovery is marked as "discovering" but no question is loaded yet Frontend: - Added "Preparing discovery questions..." loading state when in discovering state but no current question is available - Prevents showing empty UI with no way to continue
Updated Review - Recent ImprovementsGreat work on the follow-up commits! I can see you have addressed several of the issues I raised in my previous review: Issues ResolvedFixed: Race Condition (Previous Major Issue #4)
Fixed: Safe Dictionary Access (Previous Major Issue #6)
Remaining Critical IssuesThese issues from my previous review still need attention: 1. Duplicate State Persistence (lead_agent.py:362-371) Still present - you are saving current_question_text twice:
Recommendation: Check if _save_discovery_state() already persists current_question_text. If yes, remove lines 366-371. If no, update _save_discovery_state() to include it. 2. Missing Error Recovery (ui/shared.py:310-317) When start_discovery() fails, you log the error but do not broadcast failure to frontend. Frontend Impact: The UI shows Starting... indefinitely if discovery startup fails. Recommendation: Add error state broadcast and handle discovery_error in the frontend WebSocket listener. 3. Token Security Logging (auth/dependencies.py:36) Debug log leaks information about auth state. Recommendation: Remove this log or move behind a debug flag check. Remaining Minor Issues4. Incomplete Docstring (websocket.py:57-60) 5. Unclear Comment (shared.py:308) 6. Package Update (package.json) Overall AssessmentThe recent commits show good responsiveness to feedback and fix important edge cases. The remaining critical issues are:
Status: Close to merge-ready once critical issues 1-2 are addressed. Issue 3 is lower priority but should be fixed before production. Great progress! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web-ui/src/components/DiscoveryProgress.tsx (2)
74-101: Error handling doesn't distinguish between submit failure and post-submit fetch failure.If the answer submission succeeds (line 66-72) but
getDiscoveryProgress(line 81) fails, the catch block shows a generic "Failed to submit answer" error. The user may re-submit an already-submitted answer.🔎 Suggested improvement
// Fetch next question immediately - const response = await projectsApi.getDiscoveryProgress(projectId); - setData(response.data); - setIsLoadingNextQuestion(false); + try { + const response = await projectsApi.getDiscoveryProgress(projectId); + setData(response.data); + } catch (fetchErr) { + console.error('Failed to fetch next question:', fetchErr); + // Answer was submitted successfully, just couldn't fetch next question + // Don't show error - WebSocket or auto-refresh will pick it up + } finally { + setIsLoadingNextQuestion(false); + } // Check if discovery just completed if (response.data.discovery?.state === 'completed') {
114-126:setIsStarting(false)infetchProgressmay cause premature state reset.
fetchProgressis called from multiple contexts (initial load, auto-refresh, WebSocket handlers). ResettingisStartinghere could prematurely clear the loading state if an auto-refresh triggers during start.🔎 Suggested fix
Remove
setIsStarting(false)fromfetchProgressand handle it explicitly inhandleStartDiscovery:const fetchProgress = useCallback(async () => { try { const response = await projectsApi.getDiscoveryProgress(projectId); setData(response.data); setError(null); } catch (err) { setError('Failed to load discovery progress'); console.error('Error fetching discovery progress:', err); } finally { setLoading(false); - setIsStarting(false); } }, [projectId]);Then in
handleStartDiscovery, resetisStartingafter the fallback fetch:setTimeout(async () => { await fetchProgress(); setIsStarting(false); }, 2000);
♻️ Duplicate comments (3)
web-ui/src/components/DiscoveryProgress.tsx (3)
409-414: Replace hardcoded green colors with semantic palette.This banner uses hardcoded color values (
bg-green-50,border-green-200,text-green-600,text-green-800) which violates the coding guidelines requiring shadcn/ui Nova's semantic color palette.As per coding guidelines, use semantic colors like
bg-card,text-foreground,border, or define asuccessvariant in your theme.🔎 Recommended fix
- <div className="flex items-center gap-2 p-4 bg-green-50 rounded-lg border border-green-200"> - <span className="text-green-600 text-lg">✓</span> - <span className="text-sm font-medium text-green-800"> + <div className="flex items-center gap-2 p-4 bg-primary/10 rounded-lg border border-primary"> + <Tick01Icon className="text-primary" size={20} /> + <span className="text-sm font-medium text-foreground"> Discovery Complete — All questions answered </span> </div>
417-423: More hardcoded green colors in PRD status container.Lines 418-419 use
bg-green-50 border-green-200which should use semantic colors.<div className={`p-4 rounded-lg border ${ prdCompleted - ? 'bg-green-50 border-green-200' + ? 'bg-primary/10 border-primary' : prdError ? 'bg-destructive/10 border-destructive' : 'bg-primary/10 border-primary' }`} data-testid="prd-generation-status">
453-460: Replace emoji and hardcoded colors in PRD completed state.Uses ✓ emoji and hardcoded
text-green-*classes. As per coding guidelines, use Hugeicons and semantic colors.🔎 Recommended fix
Add import at top:
import { Tick01Icon, Cancel01Icon } from '@hugeicons/react';Then:
) : prdCompleted ? ( <> - <span className="text-green-600 text-lg flex-shrink-0">✓</span> + <Tick01Icon className="text-primary flex-shrink-0" size={20} /> <div> - <div className="text-sm font-medium text-green-800">PRD Generated Successfully</div> - <div className="text-xs text-green-700 mt-1">Your Project Requirements Document is ready. View it in the Documents section.</div> + <div className="text-sm font-medium text-foreground">PRD Generated Successfully</div> + <div className="text-xs text-muted-foreground mt-1">Your Project Requirements Document is ready. View it in the Documents section.</div> </div> </>
🧹 Nitpick comments (6)
codeframe/agents/lead_agent.py (1)
293-316: Consider persistingcurrent_question_textin_save_discovery_state()for consistency.The
_load_discovery_state()method loadscurrent_question_textfrom the database (lines 269-270), but_save_discovery_state()doesn't save it. Instead,start_discovery()manually persists it at lines 366-371. This creates asymmetry between load and save operations.Centralizing all discovery state persistence in
_save_discovery_state()would make the code easier to maintain and reduce the risk of state inconsistency.🔎 Proposed refactor
def _save_discovery_state(self) -> None: """Save discovery state to database.""" try: # Save current state self.db.create_memory( project_id=self.project_id, category="discovery_state", key="state", value=self._discovery_state, ) # Save current question ID if exists if self._current_question_id: self.db.create_memory( project_id=self.project_id, category="discovery_state", key="current_question_id", value=self._current_question_id, ) + + # Save current question text if exists (for AI-generated questions) + if self._current_question_text: + self.db.create_memory( + project_id=self.project_id, + category="discovery_state", + key="current_question_text", + value=self._current_question_text, + ) logger.debug(f"Saved discovery state: {self._discovery_state}")Then remove the manual save at lines 366-371 in
start_discovery():self._current_question_id = "ai_question" # AI-generated question self._current_question_text = response # Store the actual question text self._save_discovery_state() - # Also save the AI question to database for progress endpoint access - self.db.create_memory( - project_id=self.project_id, - category="discovery_state", - key="current_question_text", - value=response, - ) - logger.info(f"Started discovery with AI-generated question")web-ui/src/components/DiscoveryProgress.tsx (5)
38-44: Consider consolidating PRD state into a single object.Seven separate
useStatecalls for related PRD state could be consolidated into a single state object oruseReducerfor cleaner state transitions and reduced re-render potential.🔎 Example consolidation
interface PrdState { isGenerating: boolean; completed: boolean; error: string | null; stage: string; message: string; progressPct: number; } const [prdState, setPrdState] = useState<PrdState>({ isGenerating: false, completed: false, error: null, stage: '', message: '', progressPct: 0, });
137-148: Potential duplicatefetchProgresscalls after start.The WebSocket handler (line 169) and the 2-second fallback timeout (line 140) may both call
fetchProgress, causing redundant API calls.🔎 One approach to avoid duplicate calls
const handleStartDiscovery = async () => { if (isStarting) return; setIsStarting(true); setStartError(null); const fallbackTimeoutRef = { current: null as NodeJS.Timeout | null }; try { await projectsApi.startProject(projectId); // Set fallback, but WebSocket handler can clear it if it fires first fallbackTimeoutRef.current = setTimeout(() => { fetchProgress(); }, 2000); } catch (err) { // ... } };Then in the WebSocket handler, clear the timeout if it exists.
235-248: Consider addingfetchProgressto the dependency array.Since
fetchProgressis now a stableuseCallbackreference, it can safely be included in the dependency array instead of suppressing the linter.return () => clearInterval(intervalId); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [data]); + }, [data, fetchProgress]);
302-305: Extract repeated spinner SVG into a reusable component.The same spinner SVG markup appears 5+ times (lines 302-305, 327-330, 375-378, 427-430, 471-474). Extract it to a
Spinnercomponent or use a Hugeicons loading icon.As per coding guidelines, consider using Hugeicons for icons.
🔎 Example extraction
// At top of file or in separate component const Spinner = ({ className = "h-5 w-5" }: { className?: string }) => ( <svg className={`animate-spin ${className}`} xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 24 24"> <circle className="opacity-25" cx="12" cy="12" r="10" stroke="currentColor" strokeWidth="4" /> <path className="opacity-75" fill="currentColor" d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z" /> </svg> );
397-399: Consider replacing 💡 emoji with Hugeicons.For consistency with the coding guidelines, consider using a Hugeicons icon (e.g.,
Idea01IconorLightBulbIcon) instead of the emoji.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/agents/lead_agent.pyweb-ui/src/components/DiscoveryProgress.tsx
🧰 Additional context used
📓 Path-based instructions (4)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/agents/lead_agent.py
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use functional React components with TypeScript interfaces
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use PascalCase for React component names
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
🧬 Code graph analysis (1)
codeframe/agents/lead_agent.py (3)
codeframe/persistence/database.py (1)
create_memory(502-504)codeframe/discovery/questions.py (2)
generate_questions(61-171)get_next_question(173-207)codeframe/core/project.py (1)
chat(689-700)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (5)
codeframe/agents/lead_agent.py (3)
259-259: LGTM! Clean addition of AI-generated question tracking.The new
_current_question_textfield and its persistence logic follow the existing pattern for discovery state management.Also applies to: 269-270
386-425: LGTM! Well-structured prompt engineering.The helper method builds a clear, context-aware prompt that guides Claude to ask relevant follow-up questions while avoiding redundancy with the provided project description.
501-533: LGTM! Robust handling of AI-generated and framework questions.The method correctly:
- Calculates progress metrics even when
current_question_idis None (addressing the edge case mentioned in the commit message)- Prioritizes AI-generated questions when
current_question_id == "ai_question"- Falls back to framework questions otherwise
web-ui/src/components/DiscoveryProgress.tsx (2)
8-16: LGTM!Imports are well-organized with proper type imports for WebSocket integration.
486-513: LGTM!The idle state UI correctly uses semantic color classes (
bg-muted,bg-primary,text-primary-foreground,bg-destructive/10, etc.) as per coding guidelines.
Backend fixes: - Remove duplicate state persistence in lead_agent.py (_save_discovery_state now handles question_text) - Use provider directly for discovery question generation to avoid conversation history pollution - Add discovery_error WebSocket broadcast when start_discovery() fails - Add 120s timeout to PRD generation to prevent indefinite hangs - Fix PRD key mismatch in activity_repository.py (content vs prd_content) - Remove debug log leaking auth state in dependencies.py - Update websocket.py docstring to reflect JWT validation Frontend fixes: - Separate submit failure from post-submit fetch failure in DiscoveryProgress - Fix setIsStarting premature reset using ref tracking - Replace emoji with Hugeicons (Cancel01Icon, CheckmarkCircle01Icon) - Add restartDiscovery and retryPrdGeneration API methods - Add discovery_question_ready and discovery_reset WebSocket message types Documentation: - Update discovery-flow-analysis.md with implementation status
Updated Review - All Critical Issues Resolved! ✅Great work addressing the feedback from my previous reviews! I've examined the latest commits (be0dbef and ea0d1b0) and can confirm that all critical issues have been resolved. ✅ Issues Successfully Fixed1. Duplicate State Persistence - RESOLVEDPrevious Issue: Fix: Updated Location: 2. Missing Error Recovery - RESOLVEDPrevious Issue: Discovery startup failures left frontend showing "Starting discovery" indefinitely Fix: Added comprehensive error handling with WebSocket broadcast of type Location: 3. Token Security Logging - RESOLVEDPrevious Issue: Debug logging revealed credential presence in logs Fix: Removed the debug logging statement entirely. Location: 🎯 Code Quality AssessmentThe latest changes demonstrate:
Minor ObservationsThese are minor points that don't block merging but could be addressed in follow-up work:
Test CoverageThe WebSocket JWT authentication tests are comprehensive. Consider adding these test cases in a follow-up PR:
🚀 RecommendationStatus: ✅ APPROVED - Ready to Merge All critical issues have been addressed. The minor observations above are suggestions for future improvements and should not block this PR. This is solid work that:
Great job on the iterative improvements! 🎉 Review completed on commit ea0d1b0 |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
web-ui/src/components/DiscoveryProgress.tsx (1)
577-582: Replace hardcoded green colors with semantic palette.Despite being marked as addressed in a previous review (commit be0dbef), this code still uses hardcoded Tailwind green classes (
bg-green-50,border-green-200,text-green-600,text-green-800), which violates the coding guideline requiring shadcn/ui Nova's semantic color palette.As per coding guidelines, use semantic colors like
bg-card,text-foreground,border, etc.🔎 Recommended fix using semantic colors
- <div className="flex items-center gap-2 p-4 bg-green-50 rounded-lg border border-green-200"> - <span className="text-green-600 text-lg">✓</span> - <span className="text-sm font-medium text-green-800"> + <div className="flex items-center gap-2 p-4 bg-primary/10 rounded-lg border border-primary"> + <CheckmarkCircle01Icon className="text-primary h-5 w-5" aria-hidden="true" /> + <span className="text-sm font-medium text-foreground"> Discovery Complete — All questions answered </span> </div>Based on coding guidelines requiring semantic color palette and Hugeicons for all icons.
🧹 Nitpick comments (1)
web-ui/src/components/DiscoveryProgress.tsx (1)
475-478: Update restart button colors to semantic palette.The restart button uses hardcoded amber colors (
bg-amber-600,hover:bg-amber-700) which should be semantic colors.🔎 Recommended fix
className={`px-4 py-2 rounded-lg font-medium transition-colors ${ isRestarting ? 'bg-muted cursor-not-allowed text-muted-foreground' - : 'bg-amber-600 hover:bg-amber-700 text-white' + : 'bg-destructive hover:bg-destructive/90 text-destructive-foreground' }`}Based on coding guidelines requiring semantic color palette.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.gitignorecodeframe/agents/lead_agent.pycodeframe/persistence/repositories/activity_repository.pycodeframe/ui/routers/discovery.pycodeframe/ui/routers/websocket.pycodeframe/ui/shared.pydocs/discovery-flow-analysis.mdweb-ui/src/components/DiscoveryProgress.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
📓 Path-based instructions (7)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/lib/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend API files must use const API_BASE_URL = process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8080' pattern without hardcoded production URLs or different fallback ports
Files:
web-ui/src/lib/api.ts
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/ui/routers/discovery.pycodeframe/ui/routers/websocket.pycodeframe/ui/shared.pycodeframe/agents/lead_agent.pycodeframe/persistence/repositories/activity_repository.py
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Documentation files must be sized to fit in a single agent context window (spec.md ~200-400 lines, plan.md ~300-600 lines, tasks.md ~400-800 lines)
Files:
docs/discovery-flow-analysis.md
codeframe/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use repository pattern architecture for data access with 17 domain-specific repositories instead of monolithic database class
Files:
codeframe/persistence/repositories/activity_repository.py
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/lib/api.tsdocs/discovery-flow-analysis.mdcodeframe/ui/shared.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/types/index.ts
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use functional React components with TypeScript interfaces
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use PascalCase for React component names
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Applied to files:
codeframe/ui/routers/websocket.pyweb-ui/src/types/index.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : For authentication, use FastAPI Users with JWT tokens and mandatory authentication (no bypass mode)
Applied to files:
codeframe/ui/routers/websocket.py
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/codeframe/ui/**/*.py : Use FastAPI with Uvicorn for the async API backend and WebSockets for real-time communication
Applied to files:
codeframe/ui/routers/websocket.py
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to codeframe/auth/**/*.py : Organize Python backend files with Auth module at codeframe/auth/ containing dependencies.py (get_current_user), manager.py (UserManager), models.py, router.py, and schemas.py
Applied to files:
codeframe/ui/routers/websocket.py
🧬 Code graph analysis (3)
web-ui/src/components/DiscoveryProgress.tsx (4)
web-ui/src/lib/api.ts (1)
projectsApi(30-62)web-ui/src/lib/websocket.ts (1)
getWebSocketClient(193-198)web-ui/src/types/index.ts (1)
WebSocketMessage(117-190)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (1)
WebSocketMessage(316-321)
codeframe/ui/routers/websocket.py (2)
codeframe/ui/dependencies.py (1)
get_db_websocket(50-65)codeframe/auth/manager.py (1)
get_async_session_maker(104-113)
codeframe/ui/shared.py (5)
codeframe/core/config.py (1)
get(290-302)codeframe/core/models.py (2)
project_id(234-235)ProjectStatus(34-43)codeframe/persistence/database.py (2)
get_project(278-280)update_project(286-288)codeframe/agents/lead_agent.py (1)
start_discovery(371-454)web-ui/src/types/index.ts (1)
ProjectStatus(5-5)
🪛 LanguageTool
docs/discovery-flow-analysis.md
[style] ~63-~63: ‘Without Question’ might be wordy. Consider a shorter alternative.
Context: ... Discovery State Stuck in "discovering" Without Question Location: `lead_agent.py:start_disc...
(EN_WORDINESS_PREMIUM_WITHOUT_QUESTION)
[style] ~145-~145: ‘without question’ might be wordy. Consider a shorter alternative.
Context: ...e 3**: Discovery stuck in "discovering" without question 2. Issue 1: Race condition causing ...
(EN_WORDINESS_PREMIUM_WITHOUT_QUESTION)
🪛 markdownlint-cli2 (0.18.1)
docs/discovery-flow-analysis.md
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (25)
web-ui/src/lib/api.ts (1)
54-61: LGTM!The new API methods
restartDiscoveryandretryPrdGenerationfollow the existing patterns in this file and are properly typed. The endpoints align with the backend discovery router changes mentioned in the PR summary.web-ui/src/types/index.ts (2)
103-115: LGTM!The new WebSocket message types provide comprehensive coverage of the discovery and PRD generation lifecycle. The naming is clear and consistent.
181-189: LGTM!The new optional fields are well-documented with inline comments indicating which events use them. This makes the interface easy to understand and maintain.
codeframe/ui/routers/websocket.py (3)
35-46: LGTM!The WebSocket health endpoint provides a useful way for E2E tests and monitoring tools to verify WebSocket availability before attempting connections.
123-140: LGTM!The async user lookup correctly:
- Uses the async session maker for database access
- Checks both user existence and active status
- Handles exceptions gracefully with appropriate logging
- Closes the connection with specific reasons for debugging
93-122: JWT authentication implementation is secure and properly configured.The JWT validation correctly:
- Decodes and verifies the token signature with proper algorithm specification (HS256)
- Checks token expiration and handles ExpiredSignatureError
- Validates the presence of the subject claim
- Provides specific error messages for different failure modes
- Closes connections with appropriate WebSocket close codes (1008)
JWT configuration constants are properly defined in
codeframe/auth/manager.pywith environment-aware setup (AUTH_SECRETenv var with fallback), and production safety checks are in place inserver.pyto prevent default secrets in hosted deployments.web-ui/src/components/DiscoveryProgress.tsx (5)
88-109: LGTM! Improved answer submission UX.The refactored submission flow correctly:
- Shows immediate loading feedback after successful submission
- Clears the textarea to prevent re-submission
- Separates submission errors from fetch errors
- Maintains the answer in the textarea on submission failure for retry
This is a good UX improvement that provides clear feedback to users.
135-151: LGTM! Proper use of useCallback.The
fetchProgressfunction is correctly memoized withuseCallbackand theprojectIddependency, preventing unnecessary re-creation on each render while ensuring it always has the latestprojectIdvalue.
176-220: LGTM! Proper error recovery mechanisms.The
handleRestartDiscoveryandhandleRetryPrdGenerationfunctions provide users with clear paths to recover from stuck or failed states, with appropriate loading indicators and error messages.
227-261: LGTM! Robust stuck state detection.The timeout mechanism correctly identifies when discovery is stuck (30 seconds without a question) and provides a recovery path. The logic properly resets when a question arrives or discovery exits.
263-359: LGTM! Comprehensive WebSocket event handling.The WebSocket listener properly handles all discovery and PRD lifecycle events, providing immediate UI feedback for state changes. The project ID filtering (line 269) ensures events for other projects don't interfere.
codeframe/ui/routers/discovery.py (5)
27-141: LGTM! Excellent background PRD generation implementation.The background task is well-structured with:
- Timeout protection (120s) addressing past review concerns
- Multi-stage progress broadcasts for clear UX feedback
- Comprehensive error handling with failure broadcasts
- Proper async patterns using
asyncio.to_threadfor sync operationsThe staged progress (gathering_data → calling_llm → saving) provides excellent visibility into the PRD generation process.
258-261: LGTM! Clean integration of PRD generation.Triggering PRD generation as a background task after discovery completion is the correct approach. This allows the HTTP response to return immediately while PRD generation proceeds asynchronously with WebSocket progress updates.
348-368: LGTM! Good defensive programming.The use of
.get()with defaults prevents KeyError exceptions and gracefully handles edge cases. The mapping oftext→question(Line 364) properly aligns backend and frontend field names, preventing blank questions in the UI.
384-466: LGTM! Well-designed recovery mechanism.This endpoint provides a crucial recovery path for stuck discovery states. The implementation includes:
- Comprehensive validation (project exists, user access, phase check)
- State safety (prevents restart of completed discovery)
- Proper cleanup (resets both in-memory and persisted state)
- UI synchronization (broadcasts discovery_reset)
This addresses the error recovery needs identified in the discovery flow analysis.
469-542: LGTM! Completes the error recovery story.This endpoint provides the missing retry mechanism for PRD generation failures. The implementation properly:
- Validates preconditions (discovery complete, API key present)
- Reuses existing logic (calls generate_prd_background)
- Follows consistent patterns (auth checks, error handling)
Together with
/restart, this provides complete recovery paths for both discovery and PRD generation failures.codeframe/agents/lead_agent.py (5)
259-259: LGTM! Proper state persistence for AI-generated questions.The addition of
_current_question_textto both_load_discovery_stateand_save_discovery_stateensures that AI-generated questions survive restarts and can be recovered if needed. The symmetric load/save operations maintain state consistency.Also applies to: 269-270, 313-320
327-369: LGTM! Clean recovery mechanism for stuck discovery.The
reset_discoverymethod provides a safe way to recover from stuck states:
- Preserves data (keeps answered questions for potential reuse)
- Complete cleanup (resets both in-memory and database state)
- Clear semantics (resets to idle, allowing fresh start)
- Good error handling (logs and re-raises exceptions)
This complements the
/restartendpoint in discovery.py.
371-454: LGTM! Excellent AI-driven discovery initialization.This revamp delivers intelligent, context-aware discovery:
Strengths:
- Context-aware questions using optional project_description
- Provider direct call (Line 418) addresses past review concerns by NOT using
self.chat(), keeping discovery prompts out of conversation history- Comprehensive fallback (AI → framework → default) ensures robustness
- Token tracking for observability
- Consistent state persistence in all code paths
The decision to use
provider.send_messagedirectly is the correct approach here, as noted in past reviews.
456-495: LGTM! Clear and effective prompt engineering.The prompt construction is well thought out:
- Adaptive logic adjusts instructions based on whether description exists
- Clear goals lists the discovery objectives upfront
- Focused guidance requests ONE question to avoid overwhelming users
- Format instructions ensure concise, conversational output
This should generate high-quality, context-appropriate first questions.
589-613: LGTM! Robust status reporting with stuck-state detection.The enhancements make the status API aware of AI-generated questions and provide diagnostics:
- AI question support checks for
ai_questionanddefault_questionIDs first- Framework fallback handles traditional framework questions
- Stuck-state detection (Lines 607-613) identifies when discovery is "discovering" but has no question
- Recovery signal provides
needs_recoveryflag for UI to show recovery actionsThis enables the frontend to detect and recover from stuck states.
docs/discovery-flow-analysis.md (1)
1-180: LGTM! Valuable discovery flow documentation.This analysis document provides excellent context for the PR changes:
- Implementation tracking shows P0 and P1 issues have been addressed
- State machine clarity helps visualize the discovery/PRD lifecycle
- Issue descriptions provide clear problem statements and fixes
- Prioritization guides future work
This serves as helpful reference material for understanding the complexity of the discovery flow and the rationale behind the implementation changes.
codeframe/ui/shared.py (3)
283-285: LGTM! Good idempotent design.Changing from raising
ValueErrorto returning early makesstart_agentidempotent, which is the correct design. This prevents errors in race conditions where multiple requests might attempt to start the same agent concurrently. The INFO log level is appropriate for expected behavior.
301-348: LGTM! Robust discovery integration with excellent error handling.The discovery initialization during agent startup is well-designed:
- Context propagation passes project_description to
start_discoveryfor intelligent first question- Immediate feedback broadcasts
discovery_question_readywhen first question is available- Graceful degradation tolerates discovery failures and broadcasts
discovery_errorfor UI recovery- Non-blocking allows agent startup to succeed even if discovery fails (Line 347)
- Defensive broadcasts wraps all broadcast calls in try/except to prevent cascade failures
This resilience ensures the agent starts successfully and allows manual discovery retry on failures.
350-353: LGTM! Proper phase initialization.Updating both
statustoRUNNINGandphaseto"discovery"in a single database call is efficient and aligns with the PR objectives. This ensures the project enters the discovery phase immediately after agent startup.
- Move success message outside isDiscovering block so it shows when discovery completes immediately after answering final question - Fix test timing: advance timers by 2000ms to match component's 2-second fallback refresh timeout - Add proper waitFor for success message auto-dismiss verification Fixes 12 failing frontend unit tests.
Follow-up Code ReviewGreat work addressing the previous review feedback! I can confirm that most of the critical and major issues have been resolved: ✅ Issues Resolved
🟡 Remaining Minor Issues1. Syntax Error in Comment (DiscoveryProgress.tsx:180)Missing second slash in comment: / Note: isStarting is cleared by fetchProgress() when data arrives (via clearIsStartingOnFetch ref)Should be: // Note: isStarting is cleared by fetchProgress() when data arrives (via clearIsStartingOnFetch ref)File: 2. Issue #4 (Race Condition) - Partially AddressedThe
Recommendation: Add to the error handler in } catch (err) {
setError('Failed to load discovery progress');
console.error('Error fetching discovery progress:', err);
// Clear isStarting if this was triggered by handleStartDiscovery
if (clearIsStartingOnFetch.current) {
setIsStarting(false);
clearIsStartingOnFetch.current = false;
}
}3. Issue #6 (Field Mapping) - Still PresentThe Recommendation (optional, not blocking): Consider creating a response model: class CurrentQuestionResponse(BaseModel):
id: str
question: str
category: str🧪 Testing ObservationsThe PR description mentions:
Recommendation: Before merging, please complete the manual test checklist:
📊 Code Quality AssessmentOverall: 🟢 Good to merge after fixing the comment syntax error The changes demonstrate:
Priority Recommendations
The core functionality is solid and the security improvements (JWT auth) are well-implemented. Nice work! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/src/components/DiscoveryProgress.tsx (1)
467-468: Styling issues previously flagged remain unaddressed.Several styling guideline violations that were flagged in previous reviews are still present:
- Lines 467-468, 599-604: Hardcoded amber and green colors (
bg-amber-50,border-amber-300,bg-green-50,border-green-200) should use semantic palette tokens.- Lines 474, 592: Emoji icons ("
⚠️ ", "✓") should be replaced with Hugeicons components.- Lines 637-640: Hardcoded green text colors (
text-green-600,text-green-800,text-green-700) should use semantic text colors.These issues were previously identified and marked as addressed in earlier commits, but the violations persist in the current code.
As per coding guidelines:
- Use semantic color palette (bg-card, text-foreground, border, etc.)
- Use Hugeicons (@hugeicons/react) for all icons instead of emoji
Also applies to: 474-476, 591-596, 599-604, 637-640
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web-ui/__tests__/integration/discovery-answer-flow.test.tsxweb-ui/src/components/DiscoveryProgress.tsxweb-ui/src/components/__tests__/DiscoveryProgress.test.tsx
🧰 Additional context used
📓 Path-based instructions (3)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/__tests__/DiscoveryProgress.test.tsxweb-ui/src/components/DiscoveryProgress.tsx
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/__tests__/DiscoveryProgress.test.tsxweb-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/__tests__/DiscoveryProgress.test.tsxweb-ui/src/components/DiscoveryProgress.tsx
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/__tests__/DiscoveryProgress.test.tsxweb-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use functional React components with TypeScript interfaces
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use PascalCase for React component names
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/contexts/AgentStateContext.ts : Use context-based state management with React Context + useReducer pattern for Dashboard with AgentStateContext, agentReducer, and useAgentState hook
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use Tailwind utility classes for styling instead of CSS modules
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
🧬 Code graph analysis (1)
web-ui/src/components/DiscoveryProgress.tsx (3)
web-ui/src/lib/api.ts (1)
projectsApi(30-62)web-ui/src/lib/websocket.ts (1)
getWebSocketClient(193-198)web-ui/src/types/index.ts (1)
WebSocketMessage(117-190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (2)
web-ui/src/components/__tests__/DiscoveryProgress.test.tsx (1)
498-499: The 2-second timeout change is correct and matches the component implementation.The component's
handleStartDiscoveryfunction (lines 170-174 in DiscoveryProgress.tsx) includes a 2000ms fallback timeout that callsfetchProgress()when the WebSocket is slow. The test correctly exercises this timeout by advancing timers by 2000ms and verifying thatgetDiscoveryProgressis called as the fallback refresh.web-ui/__tests__/integration/discovery-answer-flow.test.tsx (1)
165-170: No action needed—the timing and auto-dismiss behavior are correct as written.The component's auto-dismiss timeout is 1000ms (confirmed at line 96-99 in DiscoveryProgress.tsx:
setTimeout(() => { setShowSuccessMessage(false); }, 1000)). The test correctly advances timers by 1000ms at line 167 to trigger this timeout. The comment at line 165 accurately states "auto-dismiss after 1 second," and the cumulative 2000ms advancement (1000ms at line 151 + 1000ms at line 167) serves different purposes—the first allows the next question fetch to complete, the second allows the success message timeout to fire. There is no timing discrepancy.However, tests 2 and 4 (lines 259-263 and 477) could add explicit auto-dismiss verification for consistency, though this is optional and not required for correctness.
Likely an incorrect or invalid review comment.
Design system improvements: - Add --warning and --success CSS variables to globals.css (light/dark) - Add warning/success colors to tailwind.config.ts for semantic usage - Replace hardcoded amber-* classes with warning semantic tokens - Replace hardcoded green-* classes with success semantic tokens - Replace emoji⚠️ /✓ with Hugeicons (Alert02Icon, CheckmarkCircle01Icon) Test fixes: - Update DiscoveryProgress tests to expect semantic color classes - Add Hugeicons mock for Jest test environment - Fix PRD test fixtures to use key="content" (matches repository query)
Follow-up Code ReviewGreat work addressing the previous feedback! I can see several improvements have been made. Here's an updated assessment: ✅ Issues Resolved
🟡 Remaining Considerations1. Token Security Logging (Low Priority)Location: The debug log if logger.isEnabledFor(logging.DEBUG):
logger.debug(f"get_current_user called, credentials present: {credentials is not None}")2. Field Mapping in Router (Architectural)Location: The text→question mapping is better, but ideally this should be in a Pydantic response model for consistency across the codebase. Not blocking, but worth considering for future refactoring. 3. PRD Timeout ValueLocation: 120 seconds seems reasonable, but consider making this configurable via environment variable for production tuning: PRD_GENERATION_TIMEOUT = int(os.getenv("PRD_GENERATION_TIMEOUT", "120"))4. Error Broadcast PatternLocation: When await manager.broadcast({
"type": "discovery_error",
"project_id": project_id,
"error": str(e)
}, project_id=project_id)This would give users immediate feedback when discovery fails to start. 📊 Overall AssessmentStatus: ✅ Approved with Minor Suggestions The critical issues from my initial review have been addressed:
The remaining items are minor improvements that can be addressed in follow-up PRs if desired. The code is production-ready. Test Coverage: 203 backend tests passing ✓ Nice work on the quick turnaround! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web-ui/src/components/DiscoveryProgress.tsx (1)
270-366: Fix WebSocketMessage type definition and remove unsafe type casting for error extraction.The error extraction logic (lines 335-337) violates TypeScript strict mode requirements. The
WebSocketMessageinterface is missing theerrorproperty that the backend actually sends, and the code compensates with an unsafe type cast (as { error?: string }). This contradicts the coding guideline requiring "100% type safety with no hardcoded types."The backend sends
prd_generation_failedwith error at the top level:{"error": "message"}, not wrapped in adataobject. Update theWebSocketMessageinterface inweb-ui/src/types/index.tsto include:error?: string;Then simplify the error extraction to:
const errorMsg = message.error || 'PRD generation failed';This removes the unnecessary
message.data?.errorcheck and eliminates the unsafe type cast.
🧹 Nitpick comments (1)
web-ui/src/components/DiscoveryProgress.tsx (1)
40-59: Consider consolidating related state into objects for better maintainability.The component now manages 15+ state variables, including 6 PRD-related states (isGeneratingPRD, prdCompleted, prdError, prdStage, prdMessage, prdProgressPct) and 5 stuck-detection states. Consider consolidating related state into objects or using a reducer pattern to improve maintainability.
🔎 Example consolidation
// Instead of 6 separate PRD states: const [prdState, setPrdState] = useState({ isGenerating: false, completed: false, error: null, stage: '', message: '', progressPct: 0 }); // Instead of 5 separate stuck-detection states: const [discoveryState, setDiscoveryState] = useState({ waitingForQuestionStart: null, isStuck: false, isRestarting: false, restartError: null });This would simplify state updates and make the component easier to reason about.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests/api/test_api_prd.pyweb-ui/__tests__/integration/discovery-answer-flow.test.tsxweb-ui/src/app/globals.cssweb-ui/src/components/DiscoveryProgress.tsxweb-ui/src/components/__tests__/DiscoveryProgress.test.tsxweb-ui/tailwind.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/components/tests/DiscoveryProgress.test.tsx
🧰 Additional context used
📓 Path-based instructions (3)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/DiscoveryProgress.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/DiscoveryProgress.tsx
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/src/app/globals.cssweb-ui/tailwind.config.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/__tests__/integration/discovery-answer-flow.test.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/components/**/*.{ts,tsx} : Use PascalCase for React component names
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/contexts/AgentStateContext.ts : Use context-based state management with React Context + useReducer pattern for Dashboard with AgentStateContext, agentReducer, and useAgentState hook
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use Tailwind utility classes for styling instead of CSS modules
Applied to files:
web-ui/src/components/DiscoveryProgress.tsxweb-ui/tailwind.config.ts
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/DiscoveryProgress.tsx
🧬 Code graph analysis (1)
web-ui/src/components/DiscoveryProgress.tsx (4)
web-ui/src/lib/api.ts (1)
projectsApi(30-62)web-ui/src/lib/websocket.ts (1)
getWebSocketClient(193-198)web-ui/src/types/index.ts (1)
WebSocketMessage(117-190)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (1)
WebSocketMessage(316-321)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (11)
tests/api/test_api_prd.py (2)
50-52: LGTM! Test fixture correctly aligned with repository changes.The update to use
key="content"properly aligns this test fixture with the repository layer'sget_prdfilter. The inline comment clearly documents the reasoning.
212-215: LGTM! Edge case test consistently updated.The large content test is correctly updated to use
key="content", maintaining consistency with the fixture changes and repository layer.web-ui/__tests__/integration/discovery-answer-flow.test.tsx (2)
11-16: LGTM! Hugeicons mock properly added.The mock implementation correctly returns test-friendly span elements with data-testid attributes, following the same pattern used in other test files and matching the icons used in the component under test.
172-177: LGTM! Auto-dismiss verification test added.The test correctly verifies the success message auto-dismiss behavior by advancing the timer and waiting for the message to disappear, which aligns with the 1-second auto-dismiss timeout in the component.
web-ui/tailwind.config.ts (1)
30-37: LGTM! Semantic color tokens added correctly.The warning and success tokens follow the established pattern with DEFAULT and foreground variants, properly referencing CSS variables. This enables consistent usage of semantic status colors across components via Tailwind classes.
Based on coding guidelines requiring semantic color palette.
web-ui/src/components/DiscoveryProgress.tsx (6)
89-116: LGTM! Improved submission flow with proper error handling.The enhanced submission flow correctly separates success handling from refresh failures. The separate try-catch ensures that if the answer submission succeeds but the progress refresh fails, the user sees the success message and the answer is cleared, avoiding confusion.
142-158: LGTM! fetchProgress properly integrated with WebSocket flow.The useCallback wrapper and ref-based
clearIsStartingOnFetchflag enable proper coordination between the polling fallback and WebSocket-driven updates, ensuring the loading state is only cleared after data arrives.
161-227: LGTM! New control functions properly implemented.All three functions (
handleStartDiscovery,handleRestartDiscovery,handleRetryPrdGeneration) follow consistent patterns with proper guard clauses, error handling, and state management. The WebSocket-first approach with polling fallback inhandleStartDiscoveryprovides good resilience.
419-423: LGTM! Success message uses semantic colors.The success message correctly uses semantic color tokens (
bg-success/10,border-success,text-success) instead of hardcoded green classes, aligning with the coding guidelines.Based on coding guidelines requiring semantic color palette.
463-516: LGTM! Stuck detection UI uses semantic colors and Hugeicons.The stuck/waiting state UI correctly uses:
- Semantic warning colors (
bg-warning/10,border-warning,text-warning,bg-warning,text-warning-foreground)- Alert02Icon from Hugeicons instead of emoji
- Proper accessibility attributes (aria-hidden)
This addresses previous review comments about replacing hardcoded amber colors and emoji icons.
Based on coding guidelines requiring semantic color palette and Hugeicons for all icons.
591-677: LGTM! PRD status UI fully uses semantic colors and Hugeicons.The PRD generation status block correctly implements:
- Semantic colors throughout (
bg-success/10,border-success,text-success,text-destructive,text-foreground,text-muted-foreground)- Hugeicons (CheckmarkCircle01Icon, Cancel01Icon) instead of emojis
- Comprehensive state handling (generating, completed, error, fallback)
- Proper accessibility with aria-hidden on icons
All previous review comments about hardcoded colors and emoji icons have been addressed.
Based on coding guidelines requiring semantic color palette and Hugeicons for all icons.
Final Code Review - PR #184This PR successfully addresses critical WebSocket authentication and discovery UX issues. The changes are well-architected and show good iterative improvement based on previous feedback. ✅ Approved - Ready to MergeKey Achievements
🎯 Architecture HighlightsWebSocket Auth Flow (codeframe/ui/routers/websocket.py:90-142)# 1. Extract token from query params
# 2. Decode JWT with SECRET/ALGORITHM/AUDIENCE validation
# 3. Async user lookup via get_async_session_maker()
# 4. Verify user exists and is_active
# 5. Accept/reject with specific close reasonsAlignment: Matches Discovery Question Mapping (codeframe/agents/lead_agent.py:360-370)# AI question mapped to framework ID for completion tracking
first_question_data = all_questions[0] # Get first framework question
self._current_question_id = first_question_data["id"] # e.g., "problem_1"
self._current_question_text = response # Store AI-generated textRoot Cause Fixed: Previously used 📋 Remaining Minor Items (Non-Blocking)1. Debug Logging ExposureLocation: logger.debug(f"get_current_user called, credentials present: {credentials is not None}")Recommendation: Move behind debug flag check to prevent log spam: if logger.isEnabledFor(logging.DEBUG):
logger.debug(f"get_current_user called, credentials present: {credentials is not None}")Impact: Low - debug logs typically disabled in production, but best practice for sensitive auth flows. 2. Missing Error Broadcast for Discovery FailureLocation: When except Exception as e:
logger.error(f"Failed to start discovery for project {project_id}: {e}")
# Continue with agent startup even if discovery failsRecommendation: Add failure broadcast for frontend feedback: await manager.broadcast({
"type": "discovery_error",
"project_id": project_id,
"error": "Failed to start discovery",
"timestamp": time.time()
}, project_id=project_id)Impact: Medium - improves UX by giving users immediate feedback when discovery auto-start fails. 3. Field Mapping PatternLocation: Text→question field mapping works correctly but could use Pydantic response model for type safety: raw_question = status.get("current_question")
if raw_question:
discovery_data["current_question"] = {
"id": raw_question.get("id", ""),
"question": raw_question.get("text", ""), # Map text → question
"category": raw_question.get("category", ""),
}Recommendation: Consider Impact: Low - current implementation is safe with 🏆 Code Quality Metrics
📝 Comparison to CLAUDE.md Standards✅ Authentication Architecture - Aligns with FastAPI Users pattern (see CLAUDE.md "Authentication Architecture") 🎉 Final VerdictStatus: ✅ APPROVED - LGTM (Looks Good To Me) This PR is production-ready. The three remaining items are non-blocking enhancements that can be addressed in follow-up PRs:
The critical bug fix in the latest commit (mapping AI questions to framework IDs) was an excellent catch and properly resolves the discovery completion issue. Confidence Level: High - comprehensive testing, proper error handling, and alignment with project architecture standards. Great work on the iterative improvements! 🚀 Manual Test Checklist (Pre-Merge)Before merging, please verify:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
codeframe/agents/lead_agent.py (2)
390-397: Validate project_description length before using in prompt.The
project_descriptionis stored and used directly in the AI prompt without length validation. Very long descriptions could:
- Consume excessive tokens in the prompt
- Cause API errors if combined prompt exceeds model limits
- Impact cost and performance
Consider adding a length check and truncation with a warning if the description is excessively long (e.g., >2000 characters).
🔎 Proposed validation
# Store project description as context for discovery if project_description and project_description.strip(): + # Validate and truncate if necessary + max_description_length = 2000 + if len(project_description) > max_description_length: + logger.warning( + f"Project description too long ({len(project_description)} chars), " + f"truncating to {max_description_length} chars" + ) + project_description = project_description[:max_description_length] + "..." + self.db.create_memory( project_id=self.project_id, category="discovery_context", key="project_description", value=project_description.strip(), )
442-458: Simplify fallback logic to reduce duplication.The fallback path at lines 442-447 and the final fallback at lines 449-458 both attempt to fetch the next question from the framework and set similar state. The logic could be consolidated into a single fallback block for better maintainability.
🔎 Proposed simplification
except Exception as e: logger.error(f"Failed to generate AI question, falling back to default: {e}") # Fallback to first question from framework next_question = self.discovery_framework.get_next_question(self._discovery_answers) if next_question: self._current_question_id = next_question["id"] self._current_question_text = next_question["text"] - self._save_discovery_state() - return next_question["text"] - - # Final fallback: use a default question with proper state tracking - # Map to framework's first question ID for proper progress tracking - default_question = "What would you like to build? Please describe the main problem you're trying to solve." - first_question = self.discovery_framework.get_next_question({}) - self._current_question_id = first_question["id"] if first_question else "problem_1" - self._current_question_text = default_question + else: + # Use default question if framework has no questions + self._current_question_id = "problem_1" + self._current_question_text = "What would you like to build? Please describe the main problem you're trying to solve." + self._save_discovery_state() # Persists state, question_id, and question_text - - logger.info("Using default discovery question as final fallback") - return default_question + logger.info(f"Using fallback question: {self._current_question_text}") + return self._current_question_text
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/agents/lead_agent.pyweb-ui/src/app/globals.css
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/agents/lead_agent.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/**/*.tsx : Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Applied to files:
web-ui/src/app/globals.css
🧬 Code graph analysis (1)
codeframe/agents/lead_agent.py (3)
codeframe/persistence/database.py (1)
create_memory(502-504)codeframe/discovery/questions.py (2)
generate_questions(61-171)get_next_question(173-207)codeframe/providers/anthropic.py (1)
send_message(55-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (7)
web-ui/src/app/globals.css (1)
23-26: Dark mode adjustments correctly applied; verify contrast ratios.The implementation follows the established pattern correctly. The dark mode values are now properly adjusted with reduced saturation and lightness (warning: 93%→74% saturation, 47%→25% lightness; success: 76%→61%, 36%→20%), mirroring the approach used for the destructive color. This addresses the previous concern about identical values between themes.
However, verify that the light mode warning color (
45 93% 47%with white foreground) meets WCAG AA contrast requirements (4.5:1 for normal text). Highly saturated amber/yellow backgrounds with white text can sometimes fall short.#!/bin/bash # Description: Generate a contrast verification report for the new color tokens # Expected: Contrast ratios should meet WCAG AA (4.5:1) or AAA (7:1) standards python3 << 'EOF' import colorsys def hsl_to_rgb(h, s, l): """Convert HSL to RGB (0-255 range)""" s, l = s / 100, l / 100 h = h / 360 r, g, b = colorsys.hls_to_rgb(h, l, s) return int(r * 255), int(g * 255), int(b * 255) def relative_luminance(r, g, b): """Calculate relative luminance for contrast ratio""" def adjust(c): c = c / 255 return c / 12.92 if c <= 0.03928 else ((c + 0.055) / 1.055) ** 2.4 return 0.2126 * adjust(r) + 0.7152 * adjust(g) + 0.0722 * adjust(b) def contrast_ratio(rgb1, rgb2): """Calculate WCAG contrast ratio between two colors""" l1 = relative_luminance(*rgb1) l2 = relative_luminance(*rgb2) lighter = max(l1, l2) darker = min(l1, l2) return (lighter + 0.05) / (darker + 0.05) # Define colors from the CSS colors = { 'light_warning_bg': (45, 93, 47), 'light_success_bg': (142, 76, 36), 'dark_warning_bg': (45, 74, 25), 'dark_success_bg': (142, 61, 20), 'white_fg': (0, 0, 98) # foreground for all } # Convert HSL to RGB white_rgb = hsl_to_rgb(*colors['white_fg']) print("WCAG Contrast Ratio Analysis") print("=" * 60) print("WCAG AA requires 4.5:1 for normal text, 3:1 for large text") print("WCAG AAA requires 7:1 for normal text, 4.5:1 for large text") print() for name, hsl in colors.items(): if name == 'white_fg': continue bg_rgb = hsl_to_rgb(*hsl) ratio = contrast_ratio(bg_rgb, white_rgb) aa_normal = "✓" if ratio >= 4.5 else "✗" aaa_normal = "✓" if ratio >= 7.0 else "✗" print(f"{name}:") print(f" HSL: {hsl}") print(f" RGB: {bg_rgb}") print(f" Contrast ratio: {ratio:.2f}:1") print(f" WCAG AA (normal text): {aa_normal}") print(f" WCAG AAA (normal text): {aaa_normal}") print() EOFAlso applies to: 50-53
codeframe/agents/lead_agent.py (6)
259-270: LGTM! Consistent state persistence pattern.The addition of
_current_question_textfollows the established pattern for loading discovery state from the database. The implementation correctly initializes the field and restores it from persisted memories.
313-320: LGTM! State persistence properly implemented.The conditional persistence of
_current_question_textis correctly implemented and follows the same pattern as other discovery state fields.
327-369: LGTM! Reset logic preserves answered questions as intended.The
reset_discovery()method correctly resets the discovery state to idle while preserving previously answered questions. The use of empty strings to clear database values (lines 357-364) follows the established pattern for memory updates in this codebase, wherecreate_memoryuses upsert semantics.
460-499: LGTM! Well-structured prompt builder with context awareness.The
_build_discovery_start_prompt()method appropriately adjusts the AI prompt based on whether a project description is provided. The instruction to avoid asking about details already explained in the description (line 488) will improve the user experience by preventing redundant questions.
594-617: LGTM! Recovery detection handles stuck discovery states.The enhanced
get_discovery_status()correctly handles edge cases:
- Lines 594-602: Looks up framework questions and overrides text for AI-generated questions
- Lines 603-610: Provides reasonable fallback for unknown question IDs
- Lines 612-617: Detects stuck states where discovery is active but no question is available
The
needs_recoveryflag (line 615) will help the UI and API endpoints handle these edge cases gracefully.
408-426: Configuration is correctly implemented.The model name
"claude-sonnet-4-20250514"is the valid Anthropic API identifier for Claude Sonnet 4 (2025). API key validation is enforced in both LeadAgent and AnthropicProvider, withload_dotenv()configured incodeframe/core/config.pyfor environment variable loading. The message format uses standard Anthropic structure (role/content), and token usage tracking is implemented viaresponse.usagefrom the API response (input_tokens and output_tokens). No issues found.
…framework Refactored AI question handling to avoid semantic mismatch: - AI-generated questions now use "ai_generated" ID (not framework IDs) - Default fallback questions use "default_generated" ID - process_discovery_answer() maps answers to the first unanswered framework question when current question is AI/default generated - get_discovery_status() handles AI question IDs for proper display - Dark mode color contrast fix for warning/success tokens (HSL adjusted) This ensures: 1. AI questions have distinct identity (no framework ID overwrite) 2. Answers are correctly stored under framework question IDs 3. is_discovery_complete() works because answers map to problem_1, users_1, etc. 4. No semantic mismatch between question asked and ID used
213e088 to
413c3ae
Compare
Updated Code Review - Latest Changes (Commits afba362 → 413c3ae)I've reviewed the 5 most recent commits that address previous feedback. Overall, excellent progress on resolving the issues I raised earlier! Here's my assessment: ✅ Issues Successfully Resolved1. AI Question ID Mapping (commit 413c3ae)Previous issue: AI-generated questions used ID "ai_question" which didn't map to framework questions. Resolution: ✅ Now uses "ai_generated" as distinct ID and properly maps answers to framework questions in 2. Success Message Visibility (commit afba362)Previous issue: Success messages disappeared during state transitions. Resolution: ✅ Separated submission state from loading state, added proper auto-dismiss logic. Clean implementation! 3. Error Handling in submitAnswer() (DiscoveryProgress.tsx:102-116)Previous issue: Missing finally block could leave Resolution: ✅ Added nested try-catch-finally to handle fetch failures separately from submission failures. Excellent defensive coding! 4. Token Usage Logging (lead_agent.py:421-426)Nice addition: Now logs token usage for discovery question generation. Helpful for cost tracking! 💡 Minor Observations (Not Blocking)1. New Semantic Color Tokens (commit 2e6f16e)Added
Currently using hardcoded colors. Not a blocker, just a consistency note. 2. Hugeicons Migration (commit 2e6f16e)Commit message mentions using Hugeicons, but I don't see icon changes in the diff. Was this for other components? 3. Discovery Reset Method (lead_agent.py:327-369)New 🎯 Overall AssessmentAll critical issues from my previous reviews have been addressed! The code is now production-ready from my perspective. Changes Since Last Review:
Test Coverage:The PR description mentions all 203 backend tests pass. Frontend builds successfully. Manual test plan is partially complete (2/4 checkboxes). Consider completing manual tests before merge:
📋 RecommendationReady to merge ✅ Minor follow-up work (can be separate PRs):
Great work addressing all the feedback! The error handling is much more robust now, and the AI question mapping solution is clean. This is solid, production-ready code. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
codeframe/agents/lead_agent.py (1)
514-541: Consider semantic mapping for AI-generated questions.The current mapping logic (lines 519-522) maps AI-generated answers to the first unanswered framework question. While this ensures answers are captured, it may create semantic mismatches. For example, if the AI asks about timeline (constraints category) but maps it to
problem_1, the answer's context is lost.Consider using lightweight NLP classification or keyword matching to map answers to the most semantically appropriate framework question category before storing.
🤖 Enhanced mapping approach
After generating the AI question, you could analyze its content to determine which framework category it relates to:
# In start_discovery, after generating AI question at line 419: question_text = response["content"] # Classify question to framework category framework_category = self._classify_question_category(question_text) # Store category hint for later mapping self.db.create_memory( project_id=self.project_id, category="discovery_state", key="ai_question_category_hint", value=framework_category, )Then in
process_discovery_answer, use the category hint to find a matching framework question:# In process_discovery_answer, at line 519: if self._current_question_id in ("ai_generated", "default_generated"): # Try to use category hint if available category_hint = self._get_category_hint() next_framework_question = self.discovery_framework.get_next_question_in_category( self._discovery_answers, preferred_category=category_hint ) # Fallback to any next question if no match in preferred category if not next_framework_question: next_framework_question = self.discovery_framework.get_next_question(self._discovery_answers)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codeframe/agents/lead_agent.pyweb-ui/src/app/globals.css
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/app/globals.css
🧰 Additional context used
📓 Path-based instructions (1)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ for backend development with FastAPI, AsyncAnthropic, SQLite with async support (aiosqlite), and tiktoken for token counting
Use token counting via tiktoken library for token budget management with ~50,000 token limit per conversation
Use asyncio patterns with AsyncAnthropic for async/await in Python backend for concurrent operations
Implement quality gates with multi-stage pre-completion checks (tests → type → coverage → review) and Git + SQLite + context snapshots for project state rollback
Use tiered memory system (HOT/WARM/COLD) with importance scoring using hybrid exponential decay algorithm for context management with 30-50% token reduction
Implement session lifecycle management with auto-save/restore using file-based storage at .codeframe/session_state.json
Files:
codeframe/agents/lead_agent.py
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
🧬 Code graph analysis (1)
codeframe/agents/lead_agent.py (4)
codeframe/persistence/database.py (1)
create_memory(502-504)codeframe/discovery/questions.py (2)
generate_questions(61-171)get_next_question(173-207)codeframe/providers/anthropic.py (1)
send_message(55-135)codeframe/discovery/answers.py (1)
capture_answer(25-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: claude-review
🔇 Additional comments (6)
codeframe/agents/lead_agent.py (6)
259-259: LGTM: Field initialization and loading.The
_current_question_textfield is properly initialized and loaded from the database, following the same pattern as other discovery state fields.Also applies to: 269-270
313-320: LGTM: Persistence logic for question text.The persistence logic for
current_question_textcorrectly mirrors the pattern used forcurrent_question_id, with appropriate conditional checks.
327-369: LGTM: Reset logic properly clears stuck state.The
reset_discovery()method correctly resets the discovery state to idle while preserving answered questions. The use of empty strings for clearing database fields is consistent with the existing pattern in this codebase.
371-456: Past issues successfully resolved.The implementation correctly addresses previous review concerns:
- Uses
provider.send_message()directly (line 418) to avoid polluting conversation history ✅- Uses distinct sentinel IDs (
ai_generated,default_generated) and defers mapping to framework questions until answer processing ✅The fallback logic is comprehensive with proper error handling and state persistence.
457-496: LGTM: Well-structured prompt builder.The prompt construction is clear and handles both scenarios (with/without project description) appropriately. The instruction to avoid redundant questions based on existing description (line 485) and to keep responses concise (line 494) are good practices.
586-638: LGTM: Comprehensive status handling with recovery detection.The logic correctly handles all question types (AI-generated, default, framework) and provides appropriate fallbacks. The stuck state detection (lines 633-638) with
needs_recoveryflag is excellent for diagnosing issues.The hardcoded category
"problem"(line 612) for AI-generated questions is acceptable for display purposes.
1 participant
Summary
Changes
WebSocket Authentication Fix
WebSocket endpoint was still using session-table authentication after the FastAPI Users migration. Now uses JWT token validation consistent with HTTP endpoints.
Discovery Process Improvements
Server Startup
.envfile at server startup (was missing ANTHROPIC_API_KEY)load_dotenv()doesn't override existing env varsOther Fixes
Test plan
Summary by CodeRabbit
New Features
New APIs / Integrations
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.