feat(e2e): Enable conditional tests and fix discovery startup#191
feat(e2e): Enable conditional tests and fix discovery startup#191
Conversation
WalkthroughLeadAgent now persists project description under Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant E2E as E2E Test / Test Utils
participant Auth as Auth Service (getAuthToken)
participant API as Backend API (/api/projects/{id}/start)
participant Lead as LeadAgent.start_discovery
participant DB as DB (memory / discovery_state)
Note over E2E,API: Optional backend-driven discovery start (conditional in createTestProject)
E2E->>Auth: request JWT for test user
Auth-->>E2E: return Bearer token
E2E->>API: POST /api/projects/{id}/start (Authorization: Bearer ...)
API->>Lead: invoke start_discovery(projectId, project_description?)
Lead->>DB: upsert discovery_state (context_project_description, current_question_*)
DB-->>Lead: ack
Lead-->>API: return success
API-->>E2E: 200 OK
Note over DB,E2E: Tests may also seed discovery_state directly for seeded-data assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![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
Fix all issues with AI Agents 🤖
In @tests/e2e/test_task_execution_flow.spec.ts:
- Around line 111-114: The assertion mixes a Locator object (agentPanel) with a
boolean (agentStatePanelVisible) so it always passes; compute a boolean for
agentPanel visibility (e.g., const agentPanelVisible = await
agentPanel.isVisible().catch(() => false) or check count and visibility) and
then assert expect(agentPanelVisible || agentStatePanelVisible).toBeTruthy(),
replacing the current expect(agentPanel || agentStatePanelVisible) call and
using the new agentPanelVisible variable.
🧹 Nitpick comments (4)
tests/e2e/seed-test-data.py (1)
1135-1135: Consider definingTABLE_MEMORYwith other table constants.For consistency with
TABLE_AGENTS,TABLE_TASKS, etc. defined at the top of the file (lines 33-38), move this constant to the same location.🔎 Suggested refactor
Add at line 39 (after other table constants):
TABLE_MEMORY = "memory"Then remove line 1135.
tests/e2e/test-utils.ts (1)
640-641: Consider replacing fixed timeout with event-based waiting.
waitForTimeout(2000)can cause flakiness in CI environments. Consider waiting for a specific condition instead.🔎 Suggested improvement
- // Wait for discovery to initialize (first question should appear) - await page.waitForTimeout(2000); + // Wait for discovery to initialize by checking for question or loading state + await page.waitForSelector( + '[data-testid="discovery-question"], [data-testid="discovery-loading"]', + { timeout: 10000 } + ).catch(() => { + // Discovery may already be complete or in different state - continue + });tests/e2e/test_task_execution_flow.spec.ts (2)
139-145: Redundant assertion after visibility checks.The final
expect(reviewPanel).toBeAttached()on line 145 is redundant since the same assertion already passed on line 133. The visibility check results (hasSummary,hasFindingsList) are captured but not used in any assertion.🔎 Suggested improvement
Either remove the redundant assertion or use the visibility results:
// Either summary or findings list should be visible (depending on data state) const hasSummary = await reviewSummary.isVisible().catch(() => false); const hasFindingsList = await reviewFindingsList.isVisible().catch(() => false); - // The review panel is attached, which is sufficient for this test - // Actual findings display depends on the backend data state - expect(reviewPanel).toBeAttached(); + // At least one of the review elements should be present + expect(hasSummary || hasFindingsList).toBeTruthy();
170-171: Fixed timeouts in tab navigation test.Using
waitForTimeout(1000)for tab content loading can cause flakiness. Consider waiting for specific content or usingwaitForLoadState.🔎 Suggested improvement
if (hasQualityGatesTab) { await qualityGatesTab.click(); - // Verify quality gates panel loads - await page.waitForTimeout(1000); + // Verify quality gates panel loads + await page.waitForLoadState('networkidle', { timeout: 5000 }).catch(() => {}); }Also applies to: 180-181
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
codeframe/agents/lead_agent.pytests/e2e/seed-test-data.pytests/e2e/test-utils.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_task_execution_flow.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Files:
tests/e2e/test-utils.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_task_execution_flow.spec.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/agents/lead_agent.py
🧠 Learnings (6)
📓 Common learnings
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/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:
tests/e2e/test-utils.tstests/e2e/test_start_agent_flow.spec.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 tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/test-utils.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_task_execution_flow.spec.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/lib/**/*.ts : 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
Applied to files:
tests/e2e/test-utils.tstests/e2e/test_start_agent_flow.spec.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/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test_task_execution_flow.spec.ts
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
tests/e2e/test_task_execution_flow.spec.ts
🧬 Code graph analysis (4)
tests/e2e/test-utils.ts (1)
tests/e2e/e2e-config.ts (1)
BACKEND_URL(11-11)
codeframe/agents/lead_agent.py (1)
codeframe/persistence/database.py (1)
create_memory(502-504)
tests/e2e/seed-test-data.py (1)
tests/test_review_api.py (1)
project_id(95-103)
tests/e2e/test_task_execution_flow.spec.ts (2)
tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)tests/e2e/test-utils.ts (4)
setupErrorMonitoring(64-104)ExtendedPage(25-27)loginUser(483-500)checkTestErrors(192-216)
⏰ 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 (16)
tests/e2e/seed-test-data.py (2)
1146-1179: Discovery state seeding implementation is well-structured.The seeding correctly:
- Clears existing entries for idempotency
- Seeds the three required fields that
LeadAgent._load_discovery_state()expects- Uses
problem_1which is the first required question inDiscoveryQuestionFramework- Includes proper error handling and count verification
1181-1193: Project phase update aligns with discovery UI requirements.Setting the project phase to
'discovery'ensures the frontend renders the discovery UI correctly when tests navigate to the seeded project.tests/e2e/test-utils.ts (3)
13-13: Import addition is appropriate.The
BACKEND_URLimport is needed for the new API call to start discovery.
565-584: Function signature update maintains backward compatibility.The
startDiscoveryparameter defaults totrue, ensuring existing callers continue to work without modification. The JSDoc updates clearly document the two-step process.
618-638: API call implementation for starting discovery is well-structured.The implementation correctly:
- Retrieves the auth token from the page context
- Includes proper Authorization header
- Handles error cases with descriptive messages
tests/e2e/test_task_execution_flow.spec.ts (1)
1-27: Well-documented test file with clear data dependencies.The header clearly documents the seeded data expectations and test coverage scope, making it easy for maintainers to understand the test requirements.
tests/e2e/test_start_agent_flow.spec.ts (4)
27-29: Conditional test pattern is well-implemented.Using
HAS_API_KEYwith a descriptive test name andtest.skip()provides clear visibility in test reports about why tests were skipped.
58-78: Seeded data verification test is appropriate.The test correctly:
- Uses the seeded project ID from environment
- Verifies the exact question text matches seed data
- Avoids any Claude API calls, ensuring it runs in CI
80-116: Conditional PRD generation test handles API dependency correctly.The test properly:
- Skips when API key is unavailable with clear messaging
- Creates a fresh project for the full discovery flow
- Handles variable discovery question counts gracefully
- Uses appropriate timeouts for Claude API latency
118-129: Redundant navigation after createTestProject.Since
createTestProjectalready navigates to the dashboard and waits for the page to load, the explicit navigation on lines 121 (per comment) is handled. However, this test doesn't have explicit navigation aftercreateTestProject, which is correct.tests/e2e/test_complete_user_journey.spec.ts (5)
29-32: Environment configuration is consistent with other test files.The pattern of using environment variables with sensible defaults is consistent across the E2E test suite.
58-102: Seeded data verification test provides good UI coverage.The test verifies:
- Authentication state is maintained
- Discovery UI renders with seeded data
- Dashboard and agent panels are accessible
- Tab navigation works correctly
This ensures basic functionality works without requiring API calls.
143-152: Discovery answer content is well-crafted for PRD generation.The detailed, multi-line answers provide meaningful context for Claude to generate a comprehensive PRD, making the test more realistic and the PRD output more useful for verification.
104-198: Full workflow test provides comprehensive E2E coverage.The conditional test properly exercises the complete user journey when the API key is available, covering project creation, discovery, PRD generation, and dashboard navigation. The skip behavior ensures CI runs remain stable.
112-112: Test IDuser-menuis confirmed to exist in the UI.The test ID is properly defined in
web-ui/src/components/Navigation.tsx(line 44) and is rendered when user authentication is successful. The assertion at line 112 is correct.codeframe/agents/lead_agent.py (1)
390-398: Fix for CHECK constraint is correct.The change from
discovery_contexttodiscovery_statecategory resolves the schema constraint violation. The database schema defines the allowed memory categories as:'pattern','decision','gotcha','preference','conversation','discovery_state','discovery_answers','prd'. The key rename tocontext_project_descriptionis appropriate for namespacing within thediscovery_statecategory and has no impact on other code (the key is written but never read from the database elsewhere).
Code Review - PR #191SummaryThis PR successfully addresses E2E test failures by fixing a critical database constraint bug and implementing a robust conditional testing strategy. Strengths1. Critical Bug Fix (lead_agent.py:390-398)
2. Test Data Seeding (seed-test-data.py:1127-1193)
3. Conditional Testing Pattern
4. Improved Test Helper (test-utils.ts:565-644)
Areas for Improvement1. Hard-coded Timeout (test-utils.ts:641)
2. Error Context (test-utils.ts:637)
3. Test Data Hardcoding (seed-test-data.py:1156)
4. Test Assertions (test_task_execution_flow.spec.ts)
Security & PerformanceSecurity: ✅ No concerns - auth properly handled Final VerdictLGTM with minor recommendations ✅ Blockers: None Successfully:
Test Results: ✅ 12 passed with API key, 10 passed + 2 skipped without Great work addressing the E2E testing gaps! |
frankbria
force-pushed
the
feat/e2e-conditional-tests
branch
from
9bfe3d7 to
acab61a
Compare
Follow-up Code Review - PR #191I've reviewed the changes in this PR along with my previous review comments and the CodeRabbit feedback. Here's my updated analysis: ✅ Excellent Improvements1. Critical Bug Fix Confirmed (lead_agent.py:390-398)
2. Robust Test Data Seeding (seed-test-data.py:1127-1193)
3. Smart Conditional Testing Pattern
🔍 Outstanding Issues to Address1. CRITICAL: Logic Error in Assertion (test_task_execution_flow.spec.ts:111-114) As CodeRabbit correctly identified, this is a type mismatch bug: // ❌ Current (always passes - mixing Locator with boolean)
expect(agentPanel || agentStatePanelVisible).toBeTruthy();Fix required: // ✅ Correct - compute boolean for agentPanel
const agentPanelVisible = await agentPanel.isVisible().catch(() => false);
expect(agentPanelVisible || agentStatePanelVisible).toBeTruthy();This is a blocker - the assertion will always pass regardless of UI state. 2. Hard-coded Timeouts (test-utils.ts:641, test_task_execution_flow.spec.ts:170-171, 180-181) Multiple instances of // ❌ Brittle
await page.waitForTimeout(2000);
// ✅ Robust
await page.waitForSelector('[data-testid="discovery-question"]', { timeout: 10000 })
.catch(() => {});3. Missing Error Context (test-utils.ts:637) throw new Error(`Failed to start discovery for project ${projectId}: ${startResponse.status()} - ${errorText}`);4. Redundant Assertion (test_task_execution_flow.spec.ts:139-145) 📊 Test Coverage AssessmentStrengths:
Weakness:
🎯 Final VerdictStatus: APPROVED with required fixes Blockers:
Recommended (non-blocking):
Test Results: ✅ 12 passed with API key, 10 passed + 2 skipped without This PR successfully:
Please address the critical assertion bug before merging. Great work on the conditional testing strategy! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/seed-test-data.py (1)
1181-1194: Consider verifying the UPDATE affected exactly one row.While the error handling is present, you could strengthen the validation by checking that exactly one row was updated.
🔎 Optional enhancement to verify UPDATE success
cursor.execute( "UPDATE projects SET phase = 'discovery' WHERE id = ?", (project_id,), ) + if cursor.rowcount != 1: + print(f"⚠️ Warning: Expected to update 1 project, but updated {cursor.rowcount}") print(f"✅ Set project {project_id} phase to 'discovery'")tests/e2e/test-utils.ts (1)
617-642: Replace fixed timeout with state-based waiting for better test reliability.Line 641 uses
page.waitForTimeout(2000)which is a fixed delay that could cause flakiness in CI environments. Consider waiting for a specific UI state or API response instead.🔎 Suggested improvement using state-based waiting
if (!startResponse.ok()) { const errorText = await startResponse.text(); throw new Error(`Failed to start discovery: ${startResponse.status()} - ${errorText}`); } - // Wait for discovery to initialize (first question should appear) - await page.waitForTimeout(2000); + // Wait for discovery to initialize by checking for the discovery question or loading state + try { + await page.getByTestId('discovery-question') + .or(page.getByTestId('discovery-loading')) + .waitFor({ state: 'visible', timeout: 5000 }); + } catch { + // Discovery might not have started yet or test doesn't need it - non-blocking + } }tests/e2e/test_start_agent_flow.spec.ts (1)
80-116: Consider replacing the fixed timeout in the answer loop with state-based waiting.Line 109 uses
page.waitForTimeout(3000)between discovery questions. While this accounts for API processing time, a state-based wait would be more reliable.🔎 Suggested improvement
// Answer the question await answerDiscoveryQuestion( page, `Test answer ${i + 1} - This is a comprehensive response to help generate the PRD.` ); - // Wait for next question or completion - await page.waitForTimeout(3000); + // Wait for next question to load or PRD to generate + try { + await page.getByTestId('discovery-question') + .or(page.getByTestId('prd-generated')) + .waitFor({ state: 'visible', timeout: 10000 }); + } catch { + // Discovery might be complete + } }tests/e2e/test_complete_user_journey.spec.ts (1)
129-152: Consider replacing the fixed timeout in the discovery answer loop.Line 151 uses
page.waitForTimeout(3000)between answering discovery questions. This is the same pattern seen in other test files and could benefit from state-based waiting for improved reliability.🔎 Suggested improvement
await answerDiscoveryQuestion(page, answer); - // Wait for processing and next question - await page.waitForTimeout(3000); + // Wait for next question to load or discovery to complete + try { + await page.getByTestId('discovery-question') + .or(page.getByTestId('prd-generated')) + .waitFor({ state: 'visible', timeout: 10000 }); + } catch { + // Discovery might be complete + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
codeframe/agents/lead_agent.pytests/e2e/seed-test-data.pytests/e2e/test-utils.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_task_execution_flow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/test_task_execution_flow.spec.ts
- codeframe/agents/lead_agent.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.tstests/e2e/test_complete_user_journey.spec.ts
🧠 Learnings (5)
📓 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 tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.tstests/e2e/test_complete_user_journey.spec.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/lib/**/*.ts : 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
Applied to files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.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/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:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.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/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test_complete_user_journey.spec.ts
🧬 Code graph analysis (3)
tests/e2e/test_start_agent_flow.spec.ts (2)
tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)tests/e2e/test-utils.ts (2)
createTestProject(580-645)answerDiscoveryQuestion(687-719)
tests/e2e/test-utils.ts (1)
tests/e2e/e2e-config.ts (1)
BACKEND_URL(11-11)
tests/e2e/seed-test-data.py (1)
tests/test_review_api.py (1)
project_id(95-103)
⏰ 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: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (8)
tests/e2e/test-utils.ts (1)
13-13: LGTM!The import of
BACKEND_URLfrom the config module follows the established pattern and enables environment-driven API endpoint configuration.tests/e2e/test_start_agent_flow.spec.ts (3)
27-29: LGTM!The environment-driven configuration pattern enables conditional test execution based on API key availability, allowing tests to run in CI without requiring Anthropic API access while still supporting full validation locally.
118-209: LGTM!The tests appropriately handle multiple discovery states and provide good coverage of edge cases. The conditional execution pattern ensures tests run reliably in both CI and local environments.
58-78: Seeded question text matches expected value.The hardcoded question text on line 77 correctly matches the value in
tests/e2e/seed-test-data.pyline 1156: both contain'What problem does this application solve?'. The test also properly uses theloginUser()helper fromtest-utils.tsfor authentication.tests/e2e/test_complete_user_journey.spec.ts (3)
20-34: LGTM!The imports and configuration constants follow the established pattern from other test files in this PR. Enabling the test suite (removing
.skipon line 34) achieves the PR objective of making these tests executable.
36-56: LGTM!The error monitoring setup provides better visibility into test failures while filtering expected transient errors. The pattern is consistent across the test suite.
104-198: Well-structured conditional test execution.The test properly gates Claude API-dependent steps behind the
HAS_API_KEYcheck and provides comprehensive coverage of the complete user journey. The test name dynamically reflects whether it will run or be skipped.tests/e2e/seed-test-data.py (1)
1127-1179: Category and question ID alignment confirmed with backend implementation.The seeding correctly uses
category = 'discovery_state'(defined in schema CHECK constraint),current_question_id = 'problem_1'(first required question in DiscoveryQuestionFramework), and the exact question text. These values align with whatLeadAgent._load_discovery_state()expects and will be properly restored from the memory table.
frankbria
force-pushed
the
feat/e2e-conditional-tests
branch
from
acab61a to
ef192ee
Compare
Final Code Review - PR #191I've reviewed the changes and my previous comments. The PR is in excellent shape! Here's my final assessment: ✅ Critical Issues - RESOLVED1. Logic Error Fixed (test_task_execution_flow.spec.ts:111-115)
✅ Excellent Improvements Confirmed1. Critical Bug Fix (lead_agent.py:390-398)
2. Robust Test Data Seeding (seed-test-data.py:1127-1193)
3. Smart Conditional Testing Pattern
4. Improved Test Helper (test-utils.ts:565-644)
💡 Minor Recommendations (Non-blocking)1. Hard-coded Timeouts
2. Error Context (test-utils.ts:637) 3. Assertion Preference 🎯 Final VerdictAPPROVED ✅ Blockers: None - all critical issues resolved Test Results: ✅ 12 passed with API key, 10 passed + 2 skipped without This PR successfully:
Excellent work on the conditional testing strategy and comprehensive E2E coverage! The seeded data approach is particularly well-designed. 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/e2e/test_start_agent_flow.spec.ts (3)
176-178: Unused variablediscoveryActive.The
discoveryActivelocator is defined but never used. This appears to be leftover code from an earlier iteration.🔎 Remove unused variable
- // At this point, discovery should be started (either auto or via button) - // Verify that we see either: - // - Discovery question input - // - Discovery in progress indicator - // - Discovery complete message - const discoveryActive = page.locator('[data-testid="discovery-answer-input"]') - .or(page.locator('text=/discovery complete/i')) - .or(page.locator('text=/not started/i')); - // Give some time for the state to update await page.waitForTimeout(1000);
141-141: Inconsistent URL construction.Lines 141 and 196 use relative paths (
/projects/${projectId}) while other navigations in this file use${FRONTEND_URL}/projects/${projectId}. This inconsistency could cause issues if Playwright's base URL configuration differs.🔎 Use consistent URL construction
- await page.goto(`/projects/${projectId}`); + await page.goto(`${FRONTEND_URL}/projects/${projectId}`);Also applies to: 196-196
109-109: Consider replacing fixed waits with explicit conditions.Multiple
waitForTimeoutcalls introduce potential flakiness. Consider replacing with explicit waits for state changes where possible.For example, instead of
await page.waitForTimeout(3000)after answering a question, wait for the next question or completion state:await Promise.race([ page.getByTestId('discovery-question').waitFor({ state: 'visible', timeout: 10000 }), page.getByTestId('prd-generated').waitFor({ state: 'visible', timeout: 10000 }) ]);Also applies to: 151-152, 168-169, 181-182
tests/e2e/test_complete_user_journey.spec.ts (1)
29-32: Consider importingFRONTEND_URLfrom shared config.
FRONTEND_URLis already exported fromtests/e2e/e2e-config.ts(as shown in relevant snippets). Importing it would reduce duplication across test files.🔎 Import from shared config
import { test, expect } from '@playwright/test'; import { answerDiscoveryQuestion, loginUser, createTestProject, setupErrorMonitoring, checkTestErrors, ExtendedPage } from './test-utils'; +import { FRONTEND_URL } from './e2e-config'; // Check if API key is available for tests that require Claude API calls const HAS_API_KEY = !!process.env.ANTHROPIC_API_KEY; -const FRONTEND_URL = process.env.FRONTEND_URL || 'http://localhost:3001'; const PROJECT_ID = process.env.E2E_TEST_PROJECT_ID || '1';
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
codeframe/agents/lead_agent.pytests/e2e/seed-test-data.pytests/e2e/test-utils.tstests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_task_execution_flow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/test-utils.ts
- tests/e2e/test_task_execution_flow.spec.ts
- codeframe/agents/lead_agent.py
- tests/e2e/seed-test-data.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_checkpoint_ui.spec.ts
🧠 Learnings (4)
📓 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 tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test_complete_user_journey.spec.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/lib/**/*.ts : 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
Applied to files:
tests/e2e/test_start_agent_flow.spec.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/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:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test_checkpoint_ui.spec.ts
🧬 Code graph analysis (1)
tests/e2e/test_complete_user_journey.spec.ts (3)
tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)tests/e2e/test-utils.ts (6)
setupErrorMonitoring(64-104)ExtendedPage(25-27)loginUser(483-500)checkTestErrors(192-216)createTestProject(580-645)answerDiscoveryQuestion(687-719)codeframe/cli/discovery_commands.py (1)
answer(159-216)
⏰ 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). (3)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
🔇 Additional comments (6)
tests/e2e/test_checkpoint_ui.spec.ts (1)
255-262: LGTM!The changes correctly pass
startDiscovery = falsetocreateTestProject, aligning with the PR's goal of separating UI-only checkpoint verification from discovery startup. The inline comment clearly documents the intent.tests/e2e/test_start_agent_flow.spec.ts (2)
27-30: LGTM - Clear conditional test gating.The
HAS_API_KEYpattern provides a clean way to conditionally run tests requiring Claude API calls while allowing UI-only tests to pass in CI.
58-78: LGTM - Good use of seeded data for CI-safe testing.The test correctly uses seeded project data from
seed-test-data.pyand verifies the expected discovery question content, enabling reliable testing without API key dependency.tests/e2e/test_complete_user_journey.spec.ts (3)
34-56: LGTM - Robust test lifecycle setup.The error monitoring setup and transient error filtering provide good resilience against flaky failures from expected network behaviors (WebSocket reconnects, request cancellations during navigation).
58-102: LGTM - Comprehensive seeded-data UI verification.This test provides solid CI coverage by validating authentication, discovery UI, dashboard panels, and tab navigation using pre-seeded data—no API key required.
104-198: LGTM - Well-structured conditional full-workflow test.The test properly gates itself with
test.skip(!HAS_API_KEY, ...)and covers the complete journey from project creation through PRD generation and dashboard verification. The multi-line answer template (lines 143-146) provides meaningful content for the discovery process.One minor note: line 151 uses
waitForTimeout(3000)which could be replaced with an explicit state wait (similar to the suggestion intest_start_agent_flow.spec.ts), but this is acceptable given the inherent variability of Claude API response times.
frankbria
force-pushed
the
feat/e2e-conditional-tests
branch
from
ef192ee to
40e8572
Compare
Final Code Review - PR #191 ✅I've reviewed the latest changes and confirmed that all critical issues have been resolved. This PR is ready to merge. ✅ Critical Bug Fixed - VERIFIEDAssertion Logic Error (test_task_execution_flow.spec.ts:111-115)
✅ Core Improvements - All Excellent1. Critical Backend Bug Fix (lead_agent.py:390-398)
2. Robust Test Data Seeding (seed-test-data.py:1127-1193)
3. Smart Conditional Testing Pattern
4. Improved Test Helper (test-utils.ts:565-644)
💡 Minor Observations (Non-blocking)These are minor points for potential future improvements, not blockers:
🎯 Final VerdictAPPROVED ✅ - Ready to Merge Test Results: ✅ 12 passed with API key, 10 passed + 2 skipped without This PR successfully:
The seeded data approach is particularly well-designed and will serve as a good pattern for future E2E tests. Excellent work! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @tests/e2e/test_complete_user_journey.spec.ts:
- Around line 29-32: Replace the local FRONTEND_URL constant in
test_complete_user_journey.spec.ts with the exported one from ./e2e-config:
remove the local declaration "const FRONTEND_URL = process.env.FRONTEND_URL ||
'http://localhost:3001';" and import FRONTEND_URL from "./e2e-config" at the top
of the file so the test uses the single shared constant used by
playwright.config.ts.
🧹 Nitpick comments (3)
tests/e2e/test-utils.ts (1)
617-644: Consider timeout and error handling improvements.The discovery start logic correctly implements auth-gated API calls with proper error handling. Consider two optional improvements:
- Request timeout: The POST request lacks an explicit timeout. In CI/slow network conditions, this could hang indefinitely.
- Response parsing safety: The error text extraction (line 636) assumes text response, which could fail for binary error responses.
🔎 Suggested improvements
// Call the start endpoint to initialize LeadAgent and begin discovery const startResponse = await page.request.post( `${BACKEND_URL}/api/projects/${projectId}/start`, { headers: { 'Authorization': `Bearer ${authToken}`, 'Content-Type': 'application/json' - } + }, + timeout: 30000 // 30s timeout for API call } ); if (!startResponse.ok()) { - const errorText = await startResponse.text(); + const errorText = await startResponse.text().catch(() => 'Unable to read error response'); throw new Error(`Failed to start discovery: ${startResponse.status()} - ${errorText}`); }tests/e2e/test_complete_user_journey.spec.ts (2)
47-55: Consider narrowing the 'discovery' error filter pattern.The pattern
'discovery'at line 54 is very broad and could suppress legitimate discovery-related errors (e.g., "Discovery API failed" or "Discovery state error"). If the intent is to filter specific transient errors, use more precise patterns like'discovery WebSocket'or'discovery connection'.🔎 Example: More specific patterns
checkTestErrors(page, 'Complete user journey test', [ 'WebSocket', 'ws://', 'wss://', 'net::ERR_ABORTED', 'net::ERR_FAILED', 'Failed to fetch', 'Load request cancelled', 'cancelled', - 'discovery' + 'discovery WebSocket', // Only filter WebSocket-related discovery errors + 'discovery connection' // Only filter connection-related discovery errors ]);
130-152: Replace hardcoded timeout with condition-based waiting.The
waitForTimeout(3000)at line 151 is a Playwright anti-pattern. Instead of waiting an arbitrary 3 seconds, wait for the specific UI condition that indicates the system is ready for the next question (e.g., the next question appearing or a loading indicator disappearing).🔎 Suggested approach
await answerDiscoveryQuestion(page, answer); - // Wait for processing and next question - await page.waitForTimeout(3000); + // Wait for either next question or discovery completion + await Promise.race([ + page.getByTestId('discovery-question').waitFor({ state: 'visible', timeout: 10000 }), + page.getByTestId('prd-generated').waitFor({ state: 'visible', timeout: 10000 }) + ]).catch(() => { + // Either condition met or timeout - continue + }); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
codeframe/agents/lead_agent.pytests/e2e/seed-test-data.pytests/e2e/test-utils.tstests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_task_execution_flow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- codeframe/agents/lead_agent.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.tstests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_task_execution_flow.spec.ts
🧠 Learnings (7)
📓 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 tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_task_execution_flow.spec.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/lib/**/*.ts : 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
Applied to files:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.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/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:
tests/e2e/test_start_agent_flow.spec.tstests/e2e/test-utils.tstests/e2e/test_checkpoint_ui.spec.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/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test-utils.tstests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_task_execution_flow.spec.ts
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
tests/e2e/test_task_execution_flow.spec.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/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
tests/e2e/test_task_execution_flow.spec.ts
🧬 Code graph analysis (3)
tests/e2e/test_start_agent_flow.spec.ts (2)
tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)tests/e2e/test-utils.ts (2)
createTestProject(580-645)answerDiscoveryQuestion(687-719)
tests/e2e/test-utils.ts (1)
tests/e2e/e2e-config.ts (1)
BACKEND_URL(11-11)
tests/e2e/test_complete_user_journey.spec.ts (3)
tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)tests/e2e/test-utils.ts (6)
setupErrorMonitoring(64-104)ExtendedPage(25-27)loginUser(483-500)checkTestErrors(192-216)createTestProject(580-645)answerDiscoveryQuestion(687-719)codeframe/cli/discovery_commands.py (1)
answer(159-216)
⏰ 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). (3)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (19)
tests/e2e/seed-test-data.py (2)
1181-1194: LGTM!The project phase update is correctly implemented with parameterized queries and appropriate error handling.
1127-1180: LGTM. Question text matches framework definition.The discovery state seeding logic is well-structured with idempotent cleanup, parameterized queries, and per-entry error handling. The hard-coded question text for
problem_1("What problem does this application solve?") matches the actual discovery framework definition incodeframe/discovery/questions.py.tests/e2e/test_checkpoint_ui.spec.ts (1)
255-262: LGTM!The explicit
startDiscovery=falseparameter correctly implements the UI-only test strategy, preventing unnecessary backend discovery initialization during checkpoint UI verification.tests/e2e/test_task_execution_flow.spec.ts (6)
31-51: LGTM!Test setup and teardown hooks properly initialize error monitoring, clear auth state, and filter transient network errors.
53-71: LGTM!The test correctly verifies task panel visibility and the presence of task statistics elements.
73-97: LGTM!The test appropriately validates that task statistics display numeric values, with a clear comment acknowledging potential API vs. seeded data differences.
99-116: LGTM!The agent panel assertion correctly computes boolean visibility values for both panels before the logical OR check. The previous issue flagged in past reviews has been properly resolved.
118-147: LGTM!The test appropriately verifies review findings panel presence with a conservative assertion strategy that accommodates variable backend data states.
149-197: LGTM!The tab navigation test correctly implements conditional checks for optional tabs and verifies bidirectional navigation functionality.
tests/e2e/test_start_agent_flow.spec.ts (6)
27-30: LGTM!The API key detection and frontend URL configuration correctly enable conditional test execution for Claude-dependent flows.
58-78: LGTM!The test correctly leverages seeded discovery state to verify UI rendering without requiring live API calls, aligning with the PR's seeded-data strategy.
80-116: Verify timeout duration for PRD generation.The conditional test correctly gates Claude API-dependent flows. However, the 15-second timeout for PRD generation (line 114) may be insufficient when Claude API calls are slow or rate-limited. Consider increasing the timeout or verifying that 3 answered questions are sufficient to trigger PRD generation.
💡 Suggested adjustment
- await expect(page.getByTestId('prd-generated')).toBeVisible({ timeout: 15000 }); + await expect(page.getByTestId('prd-generated')).toBeVisible({ timeout: 30000 });
118-129: LGTM!The test correctly opts out of discovery initialization for a UI-focused agent panel verification, consistent with the seeded-data approach.
131-186: LGTM!The test appropriately handles both automatic and manual discovery initialization flows with flexible locator patterns and conditional logic. The multiple fallback strategies accommodate variable UI states.
188-215: LGTM!The test correctly verifies that the UI handles already-in-progress discovery without displaying start errors, using appropriate conditional assertions.
tests/e2e/test-utils.ts (2)
13-13: LGTM!The BACKEND_URL import enables the discovery start API call functionality.
565-584: LGTM!The function signature update maintains backward compatibility with
startDiscovery = truedefault, and the JSDoc clearly documents the two-step flow.tests/e2e/test_complete_user_journey.spec.ts (2)
104-108: Excellent conditional test pattern.The approach of using a conditional test name and explicit
test.skip()provides clear visibility in test reports. When the API key is unavailable, the test name itself explains why it's skipped, which improves the developer experience.
58-102: Well-structured seeded data test.The test effectively verifies the UI using pre-seeded data and includes defensive checks for optional elements like tabs. The approach of separating UI verification (this test) from full API-dependent workflows (the conditional test) aligns well with the test strategy described in the PR objectives.
- Fix createTestProject to call /api/projects/{id}/start endpoint
- Fix lead_agent.py CHECK constraint bug (discovery_context → discovery_state)
- Add discovery state seeding in seed-test-data.py
- Enable previously-skipped tests with conditional API key execution
- Create test_task_execution_flow.spec.ts for task management UI
- Update tests to remove redundant navigation after project creation
Tests now run conditionally based on ANTHROPIC_API_KEY availability:
- With API key: 12 passed (full discovery + PRD flow)
- Without API key: 10 passed, 2 skipped (graceful CI behavior)
frankbria
force-pushed
the
feat/e2e-conditional-tests
branch
from
40e8572 to
0360e55
Compare
Final Code Review - PR #191 ✅I've reviewed this PR and all previous review comments. The code is in excellent shape and ready to merge. SummaryThis PR successfully fixes critical E2E test failures through:
✅ Critical Issues - All Resolved1. Schema Constraint Bug (lead_agent.py:390-398)
2. Logic Error (test_task_execution_flow.spec.ts:111-115)
🎯 Excellent Implementation1. Robust Test Data Seeding (seed-test-data.py:1127-1193)
2. Smart Conditional Testing Pattern
3. Improved Test Helper (test-utils.ts:565-644)
4. Comprehensive E2E Coverage (test_task_execution_flow.spec.ts)
📊 Code Quality Assessment
💡 Minor Recommendations (Non-blocking)These are suggestions for future improvements, not blockers:
🔒 Security & PerformanceSecurity: ✅ No concerns
Performance: ✅ Good
Final VerdictAPPROVED FOR MERGE ✅ Blockers: None - all critical issues resolved Test Results:
This PR successfully:
Excellent work on the conditional testing strategy! The seeded data approach is particularly well-designed and will make the E2E test suite much more reliable in CI environments. 🎉 Review conducted by Claude Code - consistent with previous review findings |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @tests/e2e/test_task_execution_flow.spec.ts:
- Around line 144-146: Remove the duplicate assertion
expect(reviewPanel).toBeAttached() that repeats an identical check already
performed earlier in the test; locate the redundant call that follows the
comment "// The review panel is attached, which is sufficient for this test" and
delete that extra expect(reviewPanel).toBeAttached() so the test only asserts
attachment once using the existing reviewPanel variable.
🧹 Nitpick comments (4)
tests/e2e/test_task_execution_flow.spec.ts (1)
168-182: Consider replacing arbitrary timeouts with element-based waits.The use of
page.waitForTimeout(1000)on lines 171 and 181 is an anti-pattern in Playwright tests. Instead of arbitrary delays, wait for specific panel elements to be visible or for network activity to settle (e.g.,waitForLoadState('networkidle')).🔎 Proposed refactor
if (hasQualityGatesTab) { await qualityGatesTab.click(); - // Verify quality gates panel loads - await page.waitForTimeout(1000); + // Wait for quality gates panel content to load + await page.waitForLoadState('networkidle'); } // Navigate to Metrics tab (if exists) const metricsTab = page.locator('[data-testid="metrics-tab"]'); const hasMetricsTab = await metricsTab.isVisible().catch(() => false); if (hasMetricsTab) { await metricsTab.click(); - // Verify metrics content loads - await page.waitForTimeout(1000); + // Wait for metrics content to load + await page.waitForLoadState('networkidle'); }tests/e2e/test_start_agent_flow.spec.ts (3)
27-29: Consider importingFRONTEND_URLfrom./e2e-config.
e2e-config.tsexportsFRONTEND_URLwith the same fallback value. Importing it would maintain a single source of truth, consistent with howplaywright.config.tsuses this constant.🔎 Proposed fix
import { createTestProject, answerDiscoveryQuestion, loginUser, setupErrorMonitoring, checkTestErrors, ExtendedPage } from './test-utils'; +import { FRONTEND_URL } from './e2e-config'; // Check if API key is available for tests that require Claude API calls const HAS_API_KEY = !!process.env.ANTHROPIC_API_KEY; -const FRONTEND_URL = process.env.FRONTEND_URL || 'http://localhost:3001';
58-78: MissingPROJECT_IDconstant declaration.Line 61 uses
process.env.E2E_TEST_PROJECT_ID || '1'inline, but aPROJECT_IDconstant (like intest_complete_user_journey.spec.ts) would improve consistency and readability.🔎 Proposed fix
// Check if API key is available for tests that require Claude API calls const HAS_API_KEY = !!process.env.ANTHROPIC_API_KEY; const FRONTEND_URL = process.env.FRONTEND_URL || 'http://localhost:3001'; +const PROJECT_ID = process.env.E2E_TEST_PROJECT_ID || '1';Then update line 61:
- const projectId = process.env.E2E_TEST_PROJECT_ID || '1'; + const projectId = PROJECT_ID;
140-141: Consider usingFRONTEND_URLfor consistency with other navigation calls.Line 141 uses a relative URL
/projects/${projectId}while line 64 uses${FRONTEND_URL}/projects/${projectId}. Using the same pattern throughout the file improves consistency.🔎 Proposed fix
- await page.goto(`/projects/${projectId}`); + await page.goto(`${FRONTEND_URL}/projects/${projectId}`);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
codeframe/agents/lead_agent.pytests/e2e/seed-test-data.pytests/e2e/test-utils.tstests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.tstests/e2e/test_task_execution_flow.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/e2e/test-utils.ts
- tests/e2e/seed-test-data.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/e2e/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Files:
tests/e2e/test_task_execution_flow.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_start_agent_flow.spec.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/agents/lead_agent.py
🧠 Learnings (7)
📓 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 tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/test_task_execution_flow.spec.tstests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.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/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test_task_execution_flow.spec.tstests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
tests/e2e/test_task_execution_flow.spec.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/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
tests/e2e/test_task_execution_flow.spec.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/lib/**/*.ts : 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
Applied to files:
tests/e2e/test_complete_user_journey.spec.tstests/e2e/test_start_agent_flow.spec.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/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:
tests/e2e/test_checkpoint_ui.spec.tstests/e2e/test_start_agent_flow.spec.ts
🧬 Code graph analysis (3)
codeframe/agents/lead_agent.py (1)
codeframe/persistence/database.py (1)
create_memory(502-504)
tests/e2e/test_complete_user_journey.spec.ts (2)
tests/e2e/test-utils.ts (5)
setupErrorMonitoring(64-104)ExtendedPage(25-27)loginUser(483-500)checkTestErrors(192-216)createTestProject(580-645)tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)
tests/e2e/test_start_agent_flow.spec.ts (2)
tests/e2e/e2e-config.ts (1)
FRONTEND_URL(14-14)tests/e2e/test-utils.ts (2)
createTestProject(580-645)answerDiscoveryQuestion(687-719)
⏰ 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 (13)
tests/e2e/test_task_execution_flow.spec.ts (4)
1-29: LGTM! Well-documented test structure.The imports and configuration follow the E2E testing guidelines correctly. The header documentation clearly describes the test coverage, seeded data expectations, and authentication approach.
31-51: LGTM! Proper test setup with error handling.The beforeEach and afterEach hooks correctly implement test isolation, error monitoring, and transient error filtering for network-related issues common in E2E tests.
53-97: LGTM! Thorough task statistics validation.These tests appropriately separate concerns: the first verifies element presence, the second validates numeric content using regex patterns. The comment acknowledging potential API data variance is a good practice.
99-116: LGTM! Previous logic error has been fixed.The agent panel visibility check now correctly computes boolean values for both panels before the assertion. This resolves the issue flagged in the previous review where a Locator object was mixed with a boolean.
codeframe/agents/lead_agent.py (1)
390-398: LGTM! Correct fix for the CHECK constraint violation.The change from
discovery_contexttodiscovery_statecategory aligns with the database schema's allowed values. The key rename tocontext_project_descriptionclearly indicates this is context data within the discovery state.tests/e2e/test_checkpoint_ui.spec.ts (1)
255-262: LGTM! Correct use of the newstartDiscoveryparameter.Passing
startDiscovery=falseis appropriate for this UI-focused checkpoint test, avoiding unnecessary API calls and making the test faster and more reliable.tests/e2e/test_start_agent_flow.spec.ts (3)
80-116: Good implementation of conditional test execution.The pattern using
HAS_API_KEY ? 'test name' : 'skip: reason'combined withtest.skip(!HAS_API_KEY, ...)provides clear visibility in test reports while properly gating API-dependent tests.
118-129: LGTM! Correct use ofstartDiscovery=falsefor UI-only test.The test verifies the agent status panel exists without requiring discovery, making it faster and suitable for CI without an API key.
188-215: Good defensive test for discovery error handling.The test correctly verifies that no error alerts appear during discovery initialization. The conditional gating with
HAS_API_KEYensures this test only runs when API calls can succeed.tests/e2e/test_complete_user_journey.spec.ts (4)
28-32: Good fix:FRONTEND_URLis now imported from./e2e-config.This addresses the previous review comment and maintains a single source of truth for the frontend URL configuration.
36-56: LGTM! Well-structured test lifecycle with error monitoring.The
beforeEach/afterEachpattern correctly sets up error monitoring and filters transient errors that are expected during E2E tests (WebSocket reconnects, navigation-cancelled requests, etc.).
58-102: LGTM! Comprehensive UI verification test using seeded data.This test provides excellent coverage of the dashboard UI without requiring API calls, making it reliable for CI environments. The defensive
.catch(() => false)pattern handles optional UI elements gracefully.
104-198: LGTM! Well-designed conditional full workflow test.The test provides comprehensive end-to-end coverage when an API key is available:
- Creates projects with unique names for isolation
- Uses meaningful discovery answers to generate quality PRDs
- Verifies multiple dashboard panels and navigation
- Handles optional UI elements defensively
The generous timeouts (15-20s) are appropriate for Claude API response times.
| // The review panel is attached, which is sufficient for this test | ||
| // Actual findings display depends on the backend data state | ||
| expect(reviewPanel).toBeAttached(); |
There was a problem hiding this comment.
Remove redundant assertion.
Line 146 duplicates the assertion already made on Line 134. Since the comment states that panel attachment is sufficient for this test, the duplicate assertion on Line 146 should be removed.
🔎 Proposed fix
- // The review panel is attached, which is sufficient for this test
- // Actual findings display depends on the backend data state
- expect(reviewPanel).toBeAttached();
+ // The review panel is attached, which is sufficient for this test
+ // Actual findings display depends on the backend data state📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The review panel is attached, which is sufficient for this test | |
| // Actual findings display depends on the backend data state | |
| expect(reviewPanel).toBeAttached(); | |
| // The review panel is attached, which is sufficient for this test | |
| // Actual findings display depends on the backend data state |
🤖 Prompt for AI Agents
In @tests/e2e/test_task_execution_flow.spec.ts around lines 144-146, Remove the
duplicate assertion expect(reviewPanel).toBeAttached() that repeats an identical
check already performed earlier in the test; locate the redundant call that
follows the comment "// The review panel is attached, which is sufficient for
this test" and delete that extra expect(reviewPanel).toBeAttached() so the test
only asserts attachment once using the existing reviewPanel variable.
1 participant
Summary
/api/projects/{id}/startendpoint after project creationdiscovery_context→discovery_state)seed-test-data.pyfor UI-only teststest_task_execution_flow.spec.tsfor task management UIProblem
Conditional tests requiring Claude API (discovery → PRD flow) were failing because:
createTestProjectcreated projects but didn't start discoverySolution
createTestProjecthelper to call the/api/projects/{id}/startendpointlead_agent.pyto use validdiscovery_statecategory instead ofdiscovery_contextstartDiscoveryparameter tocreateTestProjectfor flexibilityHAS_API_KEYpattern for conditional executionTest Results
Test plan
npx playwright test test_complete_user_journey.spec.ts test_task_execution_flow.spec.ts test_start_agent_flow.spec.ts --project=chromiumANTHROPIC_API_KEYsetSummary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.