fix(e2e): Complete checkpoint project_id fix with database path sync - #92
Conversation
Set DATABASE_PATH environment variable in Playwright config to point backend server at tests/e2e/.codeframe/state.db. Previously, the backend defaulted to production database (.codeframe/state.db), causing E2E test data seeding to be invisible to the backend. Fixes #89
- Replace findDatabasePath() with fixed TEST_DB_PATH matching Playwright config - Add initializeTestDatabase() to create directory and schema before seeding - Add checkpoint seeding to seed-test-data.py (3 checkpoints with correct project_id) - Remove API-based checkpoint seeding in favor of direct SQL inserts This completes the fix for #89 by ensuring: 1. Backend and seeding use the same test database (tests/e2e/.codeframe/state.db) 2. Checkpoints are seeded with the correct project_id from the script parameter 3. Database schema is initialized before seeding attempts Verified: GET /api/projects/1/checkpoints now returns 3 checkpoints with project_id=1
|
Note Reviews pausedUse the following commands to manage reviews:
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (2)
You can disable this status message by setting the WalkthroughThis PR refactors E2E test infrastructure to replace API-based seeding with direct Python database operations, centralizes test configuration, implements deterministic checkpoint seeding with file creation, and applies widespread code formatting normalization (quote consistency, line wrapping, spacing) across ~80+ files. Changes
Sequence Diagram(s)sequenceDiagram
participant GS as Global Setup
participant DB as Test Database
participant BE as Python Backend
participant OS as Filesystem
GS->>GS: Cleanup test environment
GS->>OS: Ensure DB directory exists
GS->>BE: Initialize schema (db.schema_ddl)
BE->>DB: Execute DDL, create tables
DB-->>BE: Schema ready
GS->>DB: Query existing projects (e2e-testing)
alt Project exists
DB-->>GS: Return project_id
else Create new
GS->>DB: Insert new project
DB-->>GS: Return project_id
end
GS->>OS: Execute seed-test-data.py
OS->>DB: Insert agents, tasks, token_usage, code_reviews
OS->>DB: Create checkpoint metadata
OS->>OS: Create checkpoints dir & files
OS->>DB: Insert checkpoint records
OS-->>GS: Seed complete, verification passed
GS->>GS: Tests ready to run
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes
Areas requiring extra attention:
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Code Review - PR #92SummaryThis PR addresses the root cause of issue #89 by fixing database path synchronization between the Playwright config and the test seeding process. The changes ensure that checkpoints are created with the correct ✅ Strengths
🔍 Potential Issues1. Backend Endpoint Still Not Fixed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/e2e/global-setup.ts (1)
26-46: Consider escaping the database path in the shell command.The database path is interpolated directly into the shell command string without escaping. While the path is currently hardcoded and safe, consider adding quotes around the path to handle edge cases where
__dirnamemight contain spaces or special characters.Apply this diff to add proper quoting:
- const initCommand = `cd ../.. && uv run python -c "from codeframe.persistence.database import Database; db = Database('${TEST_DB_PATH}'); db.initialize()"`; + const initCommand = `cd ../.. && uv run python -c "from codeframe.persistence.database import Database; db = Database('${TEST_DB_PATH.replace(/'/g, "\\'")}'); db.initialize()"`;Alternatively, pass the path as an environment variable:
- const initCommand = `cd ../.. && uv run python -c "from codeframe.persistence.database import Database; db = Database('${TEST_DB_PATH}'); db.initialize()"`; + const initCommand = `cd ../.. && DATABASE_PATH='${TEST_DB_PATH}' uv run python -c "import os; from codeframe.persistence.database import Database; db = Database(os.environ['DATABASE_PATH']); db.initialize()"`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/e2e/global-setup.ts(3 hunks)tests/e2e/playwright.config.ts(2 hunks)tests/e2e/seed-test-data.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Files:
tests/e2e/playwright.config.tstests/e2e/global-setup.ts
tests/e2e/playwright.config.ts
📄 CodeRabbit inference engine (CLAUDE.md)
tests/e2e/playwright.config.ts: Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Files:
tests/e2e/playwright.config.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Write tests using pytest with 100% async/await support for worker agent tests
Files:
tests/e2e/seed-test-data.py
tests/e2e/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Files:
tests/e2e/seed-test-data.py
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Applied to files:
tests/e2e/playwright.config.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Applied to files:
tests/e2e/playwright.config.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Applied to files:
tests/e2e/playwright.config.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/ui/server.py : Use uvicorn with FastAPI for backend server with auto-reload support and port validation
Applied to files:
tests/e2e/playwright.config.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/ui/server.py : Implement health check endpoint at GET /health for server readiness verification
Applied to files:
tests/e2e/playwright.config.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Applied to files:
tests/e2e/global-setup.tstests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/agents/worker_agent.py : Implement quality gates with 4 stages (tests → type checking → coverage → review) before task completion
Applied to files:
tests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/lib/checkpoint_manager.py : Implement checkpoint system with Git commits, SQLite backups, and context snapshots in .codeframe/checkpoints/
Applied to files:
tests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Create checkpoints before major refactors, risky changes, or at phase transitions
Applied to files:
tests/e2e/seed-test-data.py
🧬 Code graph analysis (3)
tests/e2e/playwright.config.ts (1)
ecosystem.staging.config.js (1)
path(1-1)
tests/e2e/global-setup.ts (1)
ecosystem.staging.config.js (1)
path(1-1)
tests/e2e/seed-test-data.py (1)
codeframe/cli.py (1)
checkpoint(155-160)
⏰ 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 (7)
tests/e2e/playwright.config.ts (2)
2-2: LGTM!The path module import is correctly added to support the database path construction.
89-89: Verify path consistency with global-setup.ts.The database path construction looks correct. Ensure this path exactly matches the
TEST_DB_PATHconstant intests/e2e/global-setup.ts(lines 12-13).tests/e2e/global-setup.ts (4)
12-13: LGTM!The fixed test database path constant is correctly defined and aligns with the PR objective to establish a centralized database location for E2E tests.
19-21: LGTM!The accessor function provides a consistent interface for obtaining the test database path.
56-57: LGTM!The function correctly uses the fixed test database path and adds helpful logging for debugging.
59-64: All checkpoint seeding requirements are correctly implemented.The
tests/e2e/seed-test-data.pyscript includes proper checkpoint seeding (lines 868–967) with 3 checkpoints, correctproject_idparameter passing, andINSERT OR REPLACEstatements to avoid UNIQUE constraint conflicts, as required.tests/e2e/seed-test-data.py (1)
667-688: LGTM! Clean quality gate failures construction.The
json.dumpsconstruction fortask_4_failuresis well-structured with appropriate failure details for type checking and code review gates. The trailing comma in the Python list (line 687) is good practice and will not affect the serialized JSON output.
Black and ruff --fix were both modifying files during pre-commit, causing potential conflicts. Removed black hook - run manually with 'uv run black .' when needed.
Code Review SummaryThis PR successfully addresses issue #89 by fixing the root cause of checkpoint project_id mismatch in E2E tests. The solution is well-structured and comprehensive. Strengths1. Root Cause AnalysisThe PR correctly identifies and fixes the fundamental issue: database path synchronization between test setup and backend server. The approach of using a fixed TEST_DB_PATH constant is much more reliable than the previous dynamic path lookup. 2. Database Path Synchronization (tests/e2e/global-setup.ts)
3. Direct Database Seeding (tests/e2e/seed-test-data.py)
4. Consistent ApproachThe seeding strategy (DELETE + INSERT for relationships, INSERT OR REPLACE for entities) is applied consistently across project_agents and code_reviews. Issues and Recommendations1. Security: SQL Injection Risk (HIGH PRIORITY)File: tests/e2e/global-setup.ts:39 TEST_DB_PATH is directly interpolated into shell command without sanitization. If the path contains special characters, it could lead to command injection. Recommendation: Use execFileSync instead of execSync, or properly escape the path. 2. Code Quality: Magic StringsFile: tests/e2e/seed-test-data.py Multiple table names hardcoded as strings throughout the file. Consider defining constants at module level to prevent typos and improve maintainability. 3. Error Handling: Silent FailuresFile: tests/e2e/global-setup.ts:72 Silent failures make debugging difficult. Tests will run but fail cryptically. Consider throwing the error to fail fast. 4. Performance: Redundant DELETE + COMMITFile: tests/e2e/seed-test-data.py:119-120, 843-844 Committing after each DELETE is unnecessary and slows down seeding. Move all commits to end. 5. Testing: Missing VerificationThe test plan checkbox for full E2E test suite is unchecked. Run full E2E suite before merging. 6. Code Consistency: Playwright ConfigFile: tests/e2e/playwright.config.ts:89 Uses path.join() inline instead of the TEST_DB_PATH constant. Consider extracting constant to shared config file. Additional ObservationsMassive PR SizeThis PR touches 125 files with 5,452 additions / 5,246 deletions. The checkpoint fix is actually quite small, but this PR includes large-scale refactoring. Recommendation: Consider splitting into:
VerdictApprove with minor changes The core fix is solid and addresses the root cause correctly. However, please address:
Test CoverageGood test coverage across:
Missing: Automated verification that seeding creates exactly 3 checkpoints with project_id=1. Related Issues
Great work on the root cause analysis and systematic fix! |
Removed ~700 lines of unused code: - seedAgents() - now seeded via Python script - seedTasks() - now seeded via Python script - seedTokenUsage() - now seeded via Python script - seedCheckpoints() - now seeded via Python script - seedReviews() - now seeded via Python script - Removed unused Page import All seeding is now done via seedDatabaseDirectly() which calls the Python script seed-test-data.py for direct SQLite inserts.
Code Review: Checkpoint project_id FixSummaryThis PR successfully addresses #89 by fixing the root cause of checkpoint ✅ Strengths1. Root Cause AnalysisThe PR correctly identifies and fixes the core issue:
2. Database Seeding Improvementstests/e2e/seed-test-data.py:
tests/e2e/global-setup.ts:
3. Test Data Quality
|
- Add os import for filesystem operations - Create .codeframe/checkpoints directory before seeding - Generate valid SQLite database files for each checkpoint with checkpoint_info table containing created_at, checkpoint_id, project_id - Write context snapshot JSON files with metadata for each checkpoint - Handle and log filesystem errors gracefully without blocking seeding
Code Review: PR #92 - Fix Checkpoint project_id MismatchSummaryThis PR fixes issue #89 by ensuring E2E test checkpoints have the correct project_id=1. The fix replaces API-based checkpoint seeding with direct SQL seeding and standardizes database path handling. STRENGTHS1. Root Cause Fix
2. Simplified Database Path Management
3. Proper Schema Initialization
4. Idempotent Seeding
CRITICAL ISSUES FOUNDSQL Injection Vulnerability (CRITICAL) Location: tests/e2e/seed-test-data.py:45-73 Problem: Using f-string interpolation instead of parameterized queries Fix Required: Replace f-string interpolation with parameterized queries using ? placeholders Impact: While current risk is low (hardcoded test data), this pattern could be copied to production code. Missing Error Handling (MAJOR) Location: tests/e2e/seed-test-data.py:29-78 Problems:
Fix Required: Add try/except/finally block with proper error reporting and sys.exit(1) on failure Hardcoded project_id (MAJOR) Multiple occurrences of project_id=1 without a central constant. Fix: Create tests/e2e/constants.py with TEST_PROJECT_ID = 1 RECOMMENDATIONSMUST FIX (Before Merge):
SHOULD FIX (This Sprint): APPROVAL DECISIONStatus: CHANGES REQUESTED Blockers:
Estimated effort: 2-3 hours Positive notes:
Overall: Good conceptual fix, but security issue must be resolved before merge. |
Replace execSync with spawnSync to prevent command injection attacks: - Use argument arrays instead of string interpolation for shell commands - Pass TEST_DB_PATH via sys.argv instead of string interpolation in Python code - Add proper exit code checking for spawned processes This addresses the HIGH priority security risk where paths containing special characters could lead to arbitrary command execution.
Code Review - PR #92: Fix checkpoint project_id mismatch in E2E testsSummaryThis PR addresses issue #89 by fixing the root cause of checkpoint ✅ Strengths1. Root Cause Analysis
2. Database Path Synchronization
3. Checkpoint Seeding Implementation
4. Code Quality
|
1. Magic strings (seed-test-data.py): - Add TABLE_* constants at module level - Add table_exists() helper function - Replace all hardcoded table names with constants 2. Error handling (global-setup.ts): - Fail fast by default on seed failures - Add E2E_ALLOW_SEED_FAILURE env var to opt-out 3. Redundant commits (seed-test-data.py): - Remove intermediate conn.commit() calls after DELETE - Single commit at end of seeding 4. Shared config (e2e-config.ts): - Extract TEST_DB_PATH, BACKEND_URL, FRONTEND_URL constants - Update playwright.config.ts and global-setup.ts to import from shared config
Code Review: PR #92 - Checkpoint project_id FixSummaryThis PR addresses the root cause of issue #89 by synchronizing database paths and adding checkpoint seeding to E2E tests. The implementation is generally solid with good attention to security, but there are several areas that need attention before merging. Critical Issues1. Command Injection Vulnerability Still Present (global-setup.ts:42)While the PR title mentions fixing command injection, there is still a vulnerability in initializeTestDatabase(). If TEST_DB_PATH contains shell metacharacters, this could allow code injection through sys.argv[1]. Recommend adding path validation. 2. Silent Failure in Database Seeding (global-setup.ts:88-90)The seeding function catches errors but does not throw, only warns. Tests will run with incomplete data and produce confusing failures. Recommend making seeding errors fatal since checkpoint seeding is critical to fix issue 89. High Priority Issues3. Missing Test VerificationThe test plan shows E2E tests not run. Run the full E2E suite before merging. This is especially critical given the PR changes 126 files. 4. Massive Scope Creep (126 files changed)The diff shows 126 files changed with 5543 additions and 5965 deletions. This makes the PR extremely difficult to review and increases merge conflict risk. Recommend splitting into logical chunks or explaining why all changes are necessary. 5. Hardcoded Paths (seed-test-data.py:926, 938, 950)Checkpoint paths are hardcoded with .codeframe/checkpoints/ prefix. This assumes a specific directory structure. Recommend calculating paths relative to db_path. Medium Priority Issues6. Duplicate Database InitializationinitializeTestDatabase() is defined but the database is also initialized via API call. Document why both are needed or consolidate. 7. INSERT OR REPLACE May Mask BugsEvery insert uses INSERT OR REPLACE which silently overwrites existing data. This could mask bugs where seeding is called multiple times. Recommend using pattern from project_agents (DELETE first, then INSERT). 8. Magic Numbers in Test DataToken costs are hardcoded. Add comments explaining the pricing formula or import from MODEL_PRICING in CLAUDE.md. 9. No Validation of Checkpoint File CreationThe code creates checkpoint files but does not verify they were created successfully. Add validation to check file exists after creation. Security ConsiderationsGood: Using spawnSync with argument arrays instead of shell execution Testing Gaps
PerformanceCheckpoint file creation overhead is under 50ms total - acceptable for E2E test setup. Verdict: Conditional ApprovalThe core fix (database path synchronization) is correct and addresses issue 89. However, the PR needs work before merging: Before Merging:
Once these are addressed, this PR will be ready to merge and unblock issue 85. Files Reviewed: tests/e2e/global-setup.ts, tests/e2e/seed-test-data.py, tests/e2e/playwright.config.ts, PR description and issue 89 context Estimated Review Time: 45 minutes for files examined (could not review all 126 files due to diff size limit) |
- Add cleanupTestEnvironment() to remove stale workspaces before tests - Only cleanup workspaces (not database) to avoid readonly database errors - Move database initialization before API calls - Add retry logic for project creation when workspace conflicts occur - Prefer existing e2e-test-project over creating new ones Fixes workspace conflict errors that caused E2E test failures.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/global-setup.ts (1)
142-153: LGTM: Updated setup flow with schema initialization.The global setup now properly initializes the database schema before seeding, ensuring a clean test environment. The comment correctly notes that checkpoint seeding is included.
Note: A past review comment identified unused API-based seed functions (
seedAgents,seedTasks,seedTokenUsage,seedCheckpoints,seedReviews) as dead code that should be removed. This appears to be tracked separately.
🧹 Nitpick comments (15)
codeframe/cli.py (1)
250-250: Minor formatting improvement—unrelated to PR objectives but harmless.This consolidates the error message into a single line, which is fine. Note that this formatting change is unrelated to the checkpoint project_id fix described in the PR objectives.
Consider grouping unrelated formatting changes in a separate housekeeping PR to keep this PR focused on the E2E checkpoint fix.
testsprite_tests/TC001_Project_Creation_with_Valid_Inputs.py (1)
1-288: Formatting changes look fine, but consider PR scope.The whitespace and formatting adjustments are benign and don't affect test behavior. However, this testsprite test file appears unrelated to the PR's stated objectives (fixing checkpoint project_id mismatches in E2E tests). Including unrelated formatting changes can make PRs harder to review and understand.
Consider moving formatting-only changes to a separate housekeeping PR.
testsprite_tests/TC008_Lint_Quality_Tracking_Updates_and_Visualization.py (1)
71-342: Consider refactoring repetitive form-filling logic.The same form-filling sequence (locate input → wait → fill "my_awesome_project" → wait → click button) is repeated approximately 12 times with identical values and locators. This repetition increases maintenance burden—if the form structure or XPaths change, you'll need to update many locations.
Consider extracting this into a helper function or using a loop structure if the test intentionally retries the same action multiple times.
Example refactor:
async def fill_and_submit_project_form(page, context, project_name: str, description: str, wait_time: int = 3000): """Helper to fill and submit the project creation form.""" frame = context.pages[-1] # Fill project name elem = frame.locator("xpath=html/body/main/div/div[2]/form/div/input").nth(0) await page.wait_for_timeout(wait_time) await elem.fill(project_name) # Fill description elem = frame.locator("xpath=html/body/main/div/div[2]/form/div[2]/textarea").nth(0) await page.wait_for_timeout(wait_time) await elem.fill(description) # Click submit elem = frame.locator("xpath=html/body/main/div/div[2]/form/button").nth(0) await page.wait_for_timeout(wait_time) await elem.click(timeout=5000) # Then replace repetitive blocks with: for _ in range(12): await fill_and_submit_project_form( page, context, "my_awesome_project", "This project is for testing lint quality trend chart and results table updates." )testsprite_tests/TC004_Dashboard_Real_Time_Updates_and_Visualization.py (1)
1-127: LGTM: Formatting improvements applied consistently.The changes normalize whitespace, quote styles, and multi-line formatting throughout the test. The reformatting improves readability without altering test logic or assertions.
However, note that this file tests dashboard real-time updates and is unrelated to the PR's stated objective of fixing checkpoint
project_idissues. Consider whether unrelated formatting changes should be included in focused bug-fix PRs, or deferred to a separate formatting/cleanup PR.testsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_Display_and_Resolution.py (1)
53-342: Consider refactoring repetitive form-filling sequences.The test contains ~13 nearly identical sequences that fill the same form fields with the same values. This duplication significantly impacts maintainability.
Additionally, the hard-coded
wait_for_timeout(3000)calls throughout are a testing anti-pattern—prefer waiting for specific conditions (element visibility, network idle, etc.) over fixed delays.Consider extracting the repeated logic into a helper function:
async def attempt_project_creation(page, frame, project_name: str, description: str): """Attempt to create a project with the given name and description.""" name_input = frame.locator("xpath=html/body/main/div/div[2]/form/div/input").nth(0) await name_input.wait_for(state="visible") await name_input.fill(project_name) desc_input = frame.locator("xpath=html/body/main/div/div[2]/form/div[2]/textarea").nth(0) await desc_input.wait_for(state="visible") await desc_input.fill(description) submit_button = frame.locator("xpath=html/body/main/div/div[2]/form/button").nth(0) await submit_button.wait_for(state="visible") await submit_button.click(timeout=5000)Then replace each sequence with:
frame = context.pages[-1] await attempt_project_creation( page, frame, "myawesomeproject", "This project is for testing blocker system with SYNC and ASYNC priorities." )testsprite_tests/TC006_Hierarchical_Task_Management_Display.py (1)
69-250: Consider refactoring repeated form-fill blocks to reduce duplication.The test contains 9 nearly identical blocks (lines 69-88, 90-109, 111-130, etc.) that fill the same form fields and click the same button. This duplication makes the test harder to maintain and obscures the test's intent. Consider extracting a helper function or using a retry loop if the test is handling validation errors.
Additionally, the test uses hard-coded 3-second waits (
wait_for_timeout(3000)) and brittle absolute XPath selectors. Playwright's auto-waiting and more resilient locators (role-based, test IDs, or relative selectors) would improve test stability.Example refactor pattern:
async def fill_project_form(frame, page, name: str, description: str): """Helper to fill and submit the project creation form.""" name_input = frame.locator("xpath=html/body/main/div/div[2]/form/div/input").nth(0) await name_input.fill(name) desc_textarea = frame.locator("xpath=html/body/main/div/div[2]/form/div[2]/textarea").nth(0) await desc_textarea.fill(description) submit_button = frame.locator("xpath=html/body/main/div/div[2]/form/button").nth(0) await submit_button.click(timeout=5000) # Then use in test: for attempt in range(max_retries): frame = context.pages[-1] await fill_project_form(frame, page, "testproject123", "Testing task tree display...") # Check if successful, break if soThis would reduce the ~180 lines of duplication to a much smaller, maintainable structure.
testsprite_tests/TC012_Real_Time_Chat_Interface_with_Lead_Agent.py (1)
49-342: Consider extracting the repeated form-filling logic into a helper function.The same fill-project-name → fill-description → click-button pattern is repeated ~14 times with identical values. This duplication makes the test harder to maintain and obscures the test's intent. Consider refactoring to a helper function:
async def fill_and_submit_project_form(page, context, name, description): frame = context.pages[-1] await frame.locator("xpath=html/body/main/div/div[2]/form/div/input").nth(0).fill(name) await page.wait_for_timeout(3000) frame = context.pages[-1] await frame.locator("xpath=html/body/main/div/div[2]/form/div[2]/textarea").nth(0).fill(description) await page.wait_for_timeout(3000) frame = context.pages[-1] await frame.locator("xpath=html/body/main/div/div[2]/form/button").nth(0).click(timeout=5000) await page.wait_for_timeout(3000)Then call it in a loop or as needed. This would reduce the ~300 lines of duplication to a much clearer structure.
testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py (2)
189-189: Justify or remove the 5-second sleep before cleanup.The hard-coded 5-second sleep adds unnecessary execution time and may indicate a timing-dependent or flaky test. If this sleep is required for a specific reason (e.g., waiting for background operations, network requests, or UI updates), please:
- Add a comment explaining why the sleep is necessary
- Consider using a condition-based wait instead:
- await asyncio.sleep(5) + # Wait for pending background operations to complete + await page.wait_for_load_state("networkidle", timeout=5000)If the sleep isn't needed for correctness, remove it to improve test execution time.
53-177: Consider using more resilient locators.The test uses absolute XPath expressions (e.g.,
xpath=html/body/main/div/div[2]/form/div/input) which are brittle and will break if the DOM structure changes. Consider using more semantic and maintainable locators:-elem = frame.locator("xpath=html/body/main/div/div[2]/form/div/input").nth(0) +elem = frame.get_by_label("Project Name") +# or +elem = frame.get_by_role("textbox", name="Project Name") +# or with a test ID +elem = frame.locator("[data-testid='project-name-input']")These alternatives are more resilient to UI changes and more readable.
testsprite_tests/TC010_Code_Review_Panel_Updates_on_WebSocket_Events.py (1)
66-72: Consider increasing the assertion timeout for WebSocket-based updates.A 1000ms timeout for verifying WebSocket-driven UI updates may be too aggressive and cause flaky tests, especially under CI load. Other similar tests in the codebase (e.g., TC004 in the snippets) use 30000ms for assertions. Consider aligning this timeout with the expected latency of WebSocket event propagation.
testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
72-78: Same timeout concern as TC010 - 1000ms may be insufficient.For a discovery Q&A completion workflow that involves backend processing and PRD generation, a 1000ms assertion timeout may lead to intermittent failures. Consider increasing to match the complexity of the operation being verified.
testsprite_tests/TC011_API_Client_Response_and_Type_Safety_Validation.py (1)
155-163: Same timeout concern - 1000ms may be too short for API validation assertions.Given that this test validates API client response times and type safety, the 1000ms timeout for the final assertion may not account for realistic API round-trip latencies under load. Consider increasing this timeout to reduce test flakiness.
testsprite_tests/TC003_Multi_Agent_State_Synchronization_via_WebSocket.py (1)
1-325: LGTM! Formatting-only changes.All changes are cosmetic: quote style normalization and spacing adjustments. Test logic unchanged.
Note: Lines 95-303 contain significant code duplication (repeated fill/click patterns). Consider extracting this into a helper function or loop in a future refactor.
testsprite_tests/TC008_Session_Lifecycle_Management___Save_and_Resume.py (1)
162-376: Consider extracting repeated form interactions into a helper function.The test contains 10+ nearly identical blocks that clear the project name, fill it with "my_awesome_project", fill the description, and click the create button. While this doesn't block the PR (which focuses on formatting), extracting this into a helper would significantly improve maintainability.
Example helper:
async def fill_project_form(page, frame, project_name: str, description: str): elem = frame.locator("xpath=html/body/main/div/div[2]/form/div/input").nth(0) await page.wait_for_timeout(3000) await elem.fill(project_name) elem = frame.locator("xpath=html/body/main/div/div[2]/form/div[2]/textarea").nth(0) await page.wait_for_timeout(3000) await elem.fill(description) elem = frame.locator("xpath=html/body/main/div/div[2]/form/button").nth(0) await page.wait_for_timeout(3000) await elem.click(timeout=5000)tests/e2e/seed-test-data.py (1)
984-1030: Path calculation depends on specific db_path structure.The path resolution logic assumes
db_pathis located at.codeframe/state.db, makingbase_dirthe E2E test root. While this aligns with the PR's fixedTEST_DB_PATH, consider adding a comment documenting this assumption for maintainability.The file creation logic is solid - SQLite backups contain valid schema and JSON snapshots are written with proper encoding.
Consider adding a comment:
# Convert relative paths to absolute paths based on db_dir's parent # Since paths are like ".codeframe/checkpoints/...", we go up from db_dir (.codeframe) + # NOTE: Assumes db_path is at .codeframe/state.db (per TEST_DB_PATH in playwright.config.ts) base_dir = os.path.dirname(db_dir) # Parent of .codeframe
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (119)
.pre-commit-config.yaml(1 hunks)codeframe/agents/backend_worker_agent.py(12 hunks)codeframe/agents/frontend_worker_agent.py(2 hunks)codeframe/agents/hybrid_worker.py(2 hunks)codeframe/agents/review_agent.py(18 hunks)codeframe/agents/review_worker_agent.py(2 hunks)codeframe/agents/test_worker_agent.py(2 hunks)codeframe/agents/worker_agent.py(1 hunks)codeframe/cli.py(1 hunks)codeframe/core/models.py(6 hunks)codeframe/core/project.py(3 hunks)codeframe/core/session_manager.py(2 hunks)codeframe/lib/checkpoint_manager.py(22 hunks)codeframe/lib/metrics_tracker.py(10 hunks)codeframe/lib/quality_gate_tool.py(1 hunks)codeframe/lib/quality_gates.py(3 hunks)codeframe/lib/sdk_hooks.py(7 hunks)codeframe/persistence/database.py(30 hunks)codeframe/persistence/migrations/migration_007_sprint10_review_polish.py(6 hunks)codeframe/persistence/migrations/migration_009_add_project_agents.py(3 hunks)codeframe/providers/sdk_client.py(2 hunks)codeframe/ui/models.py(5 hunks)codeframe/ui/routers/agents.py(5 hunks)codeframe/ui/routers/blockers.py(4 hunks)codeframe/ui/routers/chat.py(2 hunks)codeframe/ui/routers/checkpoints.py(15 hunks)codeframe/ui/routers/context.py(4 hunks)codeframe/ui/routers/discovery.py(2 hunks)codeframe/ui/routers/metrics.py(9 hunks)codeframe/ui/routers/projects.py(3 hunks)codeframe/ui/routers/quality_gates.py(8 hunks)codeframe/ui/routers/review.py(17 hunks)codeframe/ui/routers/session.py(1 hunks)codeframe/ui/routers/websocket.py(1 hunks)codeframe/ui/services/agent_service.py(3 hunks)codeframe/ui/shared.py(2 hunks)scripts/fix_api_schema.py(2 hunks)scripts/fix_workspace_env.py(1 hunks)tests/agents/test_agent_lifecycle.py(2 hunks)tests/agents/test_backend_worker_agent.py(28 hunks)tests/agents/test_bash_operations_migration.py(11 hunks)tests/agents/test_file_operations_migration.py(2 hunks)tests/agents/test_lead_agent_blocker_handling.py(3 hunks)tests/agents/test_review_agent.py(6 hunks)tests/api/conftest.py(2 hunks)tests/api/test_api_metrics.py(5 hunks)tests/api/test_api_session.py(3 hunks)tests/api/test_blocker_resolution_api.py(1 hunks)tests/api/test_chat_api.py(2 hunks)tests/api/test_discovery_endpoints.py(3 hunks)tests/api/test_multi_agent_api.py(4 hunks)tests/api/test_project_reviews.py(17 hunks)tests/api/test_workspace_cleanup.py(2 hunks)tests/blockers/test_blockers.py(1 hunks)tests/config/test_config.py(2 hunks)tests/e2e/e2e-config.ts(1 hunks)tests/e2e/global-setup.ts(3 hunks)tests/e2e/playwright.config.ts(2 hunks)tests/e2e/seed-test-data.py(11 hunks)tests/e2e/test_full_workflow.py(16 hunks)tests/integration/test_checkpoint_restore.py(10 hunks)tests/integration/test_dashboard_access.py(1 hunks)tests/integration/test_notification_workflow.py(4 hunks)tests/integration/test_quality_gates_integration.py(7 hunks)tests/integration/test_review_workflow.py(1 hunks)tests/integration/test_session_lifecycle.py(11 hunks)tests/lib/test_checkpoint_manager.py(21 hunks)tests/lib/test_metrics_tracker.py(18 hunks)tests/lib/test_quality_gate_tool.py(8 hunks)tests/lib/test_quality_gates.py(6 hunks)tests/lib/test_sdk_hooks.py(11 hunks)tests/persistence/test_project_agents.py(8 hunks)tests/test_review_api.py(1 hunks)tests/ui/test_deployment_mode.py(4 hunks)tests/ui/test_project_api.py(2 hunks)tests/ui/test_websocket_broadcasts.py(4 hunks)tests/workspace/test_workspace_manager_comprehensive.py(19 hunks)testsprite_tests/TC001_Project_Creation_Success.py(1 hunks)testsprite_tests/TC001_Project_Creation_with_Valid_Inputs.py(1 hunks)testsprite_tests/TC002_Project_Creation_Input_Validation.py(1 hunks)testsprite_tests/TC002_Project_Creation_Input_Validation_and_Error_Handling.py(1 hunks)testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py(1 hunks)testsprite_tests/TC003_Multi_Agent_State_Synchronization_via_WebSocket.py(1 hunks)testsprite_tests/TC004_Dashboard_Real_Time_Updates_and_Visualization.py(1 hunks)testsprite_tests/TC004_PRD_Viewer_Rendering.py(1 hunks)testsprite_tests/TC005_Hierarchical_Task_Management_Display_and_Interaction.py(1 hunks)testsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_Display_and_Resolution.py(1 hunks)testsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_and_Resolution.py(1 hunks)testsprite_tests/TC006_Hierarchical_Task_Management_Display.py(1 hunks)testsprite_tests/TC006_Hierarchical_Task_Management_and_Dependency_Visualization.py(1 hunks)testsprite_tests/TC006_Multi_Agent_Concurrent_Execution_and_Status_Updates.py(1 hunks)testsprite_tests/TC007_Code_Review_Panel_Auto_Update_and_Details_Display.py(1 hunks)testsprite_tests/TC007_Code_Review_Panel_Auto_Updates.py(1 hunks)testsprite_tests/TC007_Human_in_the_Loop_Blocker_Creation_and_Resolution.py(1 hunks)testsprite_tests/TC008_Lint_Quality_Tracking_Updates_and_Visualization.py(1 hunks)testsprite_tests/TC008_Lint_Quality_Tracking_and_Visualization.py(1 hunks)testsprite_tests/TC008_Session_Lifecycle_Management___Save_and_Resume.py(1 hunks)testsprite_tests/TC009_Discovery_Phase_QA_and_PRD_Generation_Workflow.py(1 hunks)testsprite_tests/TC009_Discovery_QA_and_PRD_Generation_Workflow.py(1 hunks)testsprite_tests/TC009_Quality_Gates_Enforcement_Before_Task_Completion.py(1 hunks)testsprite_tests/TC010_Code_Review_Panel_Updates_on_WebSocket_Events.py(1 hunks)testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py(1 hunks)testsprite_tests/TC011_API_Client_Response_and_Type_Safety.py(1 hunks)testsprite_tests/TC011_API_Client_Response_and_Type_Safety_Validation.py(1 hunks)testsprite_tests/TC011_Lint_Quality_Trend_Chart_Auto_Refresh_and_Visualization.py(1 hunks)testsprite_tests/TC012_Code_Review_and_Lint_Quality_Gate_Enforcement.py(1 hunks)testsprite_tests/TC012_Quality_Gates_Enforcement.py(1 hunks)testsprite_tests/TC012_Real_Time_Chat_Interface_with_Lead_Agent.py(1 hunks)testsprite_tests/TC013_API_Client_Endpoint_Response_and_Type_Safety.py(1 hunks)testsprite_tests/TC013_Chat_Interface_Real_Time_Messaging.py(1 hunks)testsprite_tests/TC013_Chat_Interface_Real_Time_Messaging_and_Markdown_Support.py(1 hunks)testsprite_tests/TC014_ChatInterface_Component_Message_Display.py(1 hunks)testsprite_tests/TC014_Robust_Context_Memory_Management_and_Visualization.py(1 hunks)testsprite_tests/TC015_ErrorBoundary_Component_Error_Handling.py(1 hunks)testsprite_tests/TC015_WebSocket_Client_Connection_Management_and_Message_Handling.py(1 hunks)testsprite_tests/TC016_Context_Memory_Item_List_with_Pagination.py(1 hunks)testsprite_tests/TC016_User_Input_Validation_on_Blocker_Resolution_Modal.py(1 hunks)testsprite_tests/TC017_Context_Tier_Distribution_Chart.py(1 hunks)testsprite_tests/TC017_Dashboard_Real_Time_Status_Update_Consistency.py(1 hunks)
✅ Files skipped from review due to trivial changes (60)
- testsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_and_Resolution.py
- testsprite_tests/TC002_Project_Creation_Input_Validation.py
- testsprite_tests/TC013_API_Client_Endpoint_Response_and_Type_Safety.py
- tests/ui/test_websocket_broadcasts.py
- testsprite_tests/TC008_Lint_Quality_Tracking_and_Visualization.py
- codeframe/ui/models.py
- testsprite_tests/TC013_Chat_Interface_Real_Time_Messaging.py
- codeframe/ui/routers/agents.py
- codeframe/agents/test_worker_agent.py
- testsprite_tests/TC017_Dashboard_Real_Time_Status_Update_Consistency.py
- tests/agents/test_agent_lifecycle.py
- codeframe/ui/shared.py
- tests/api/test_api_session.py
- testsprite_tests/TC007_Code_Review_Panel_Auto_Updates.py
- codeframe/agents/hybrid_worker.py
- tests/integration/test_session_lifecycle.py
- tests/agents/test_file_operations_migration.py
- testsprite_tests/TC012_Code_Review_and_Lint_Quality_Gate_Enforcement.py
- codeframe/core/models.py
- testsprite_tests/TC015_WebSocket_Client_Connection_Management_and_Message_Handling.py
- codeframe/ui/routers/discovery.py
- tests/blockers/test_blockers.py
- testsprite_tests/TC015_ErrorBoundary_Component_Error_Handling.py
- testsprite_tests/TC014_Robust_Context_Memory_Management_and_Visualization.py
- testsprite_tests/TC014_ChatInterface_Component_Message_Display.py
- codeframe/agents/frontend_worker_agent.py
- tests/integration/test_review_workflow.py
- tests/integration/test_checkpoint_restore.py
- scripts/fix_workspace_env.py
- codeframe/core/project.py
- tests/api/test_multi_agent_api.py
- codeframe/ui/routers/projects.py
- tests/test_review_api.py
- codeframe/lib/quality_gate_tool.py
- codeframe/ui/services/agent_service.py
- testsprite_tests/TC009_Discovery_Phase_QA_and_PRD_Generation_Workflow.py
- tests/api/test_api_metrics.py
- tests/agents/test_lead_agent_blocker_handling.py
- testsprite_tests/TC011_API_Client_Response_and_Type_Safety.py
- tests/lib/test_sdk_hooks.py
- testsprite_tests/TC005_Hierarchical_Task_Management_Display_and_Interaction.py
- testsprite_tests/TC009_Discovery_QA_and_PRD_Generation_Workflow.py
- tests/agents/test_bash_operations_migration.py
- tests/lib/test_metrics_tracker.py
- codeframe/ui/routers/chat.py
- tests/persistence/test_project_agents.py
- codeframe/ui/routers/websocket.py
- testsprite_tests/TC009_Quality_Gates_Enforcement_Before_Task_Completion.py
- testsprite_tests/TC016_User_Input_Validation_on_Blocker_Resolution_Modal.py
- tests/agents/test_review_agent.py
- testsprite_tests/TC007_Code_Review_Panel_Auto_Update_and_Details_Display.py
- tests/integration/test_notification_workflow.py
- tests/api/test_project_reviews.py
- tests/api/test_workspace_cleanup.py
- tests/config/test_config.py
- testsprite_tests/TC004_PRD_Viewer_Rendering.py
- codeframe/lib/checkpoint_manager.py
- codeframe/ui/routers/context.py
- codeframe/ui/routers/metrics.py
- codeframe/persistence/database.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/playwright.config.ts
🧰 Additional context used
📓 Path-based instructions (9)
codeframe/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/**/*.py: Use Python 3.11+ with async/await patterns for backend development
Store context items in SQLite with aiosqlite for async database operations
Use snake_case for variable and function names in Python code
Run ruff linter on Python code using 'ruff check .' command
Use async context managers (async with) for database connections in Python
Files:
codeframe/lib/metrics_tracker.pycodeframe/persistence/migrations/migration_009_add_project_agents.pycodeframe/agents/worker_agent.pycodeframe/agents/backend_worker_agent.pycodeframe/ui/routers/session.pycodeframe/agents/review_worker_agent.pycodeframe/ui/routers/review.pycodeframe/ui/routers/blockers.pycodeframe/lib/quality_gates.pycodeframe/ui/routers/quality_gates.pycodeframe/core/session_manager.pycodeframe/cli.pycodeframe/lib/sdk_hooks.pycodeframe/persistence/migrations/migration_007_sprint10_review_polish.pycodeframe/ui/routers/checkpoints.pycodeframe/agents/review_agent.pycodeframe/providers/sdk_client.py
codeframe/lib/metrics_tracker.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/lib/metrics_tracker.py: Use model pricing constants for claude-sonnet-4-5, claude-opus-4, and claude-haiku-4 in cost calculations
Record token usage with tracking of model_name, input_tokens, output_tokens, call_type, task_id, agent_id, and project_id
Files:
codeframe/lib/metrics_tracker.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Write tests using pytest with 100% async/await support for worker agent tests
Files:
tests/e2e/test_full_workflow.pytests/ui/test_deployment_mode.pytests/api/conftest.pytests/api/test_discovery_endpoints.pytests/lib/test_quality_gates.pytests/ui/test_project_api.pytests/api/test_blocker_resolution_api.pytests/integration/test_dashboard_access.pytests/integration/test_quality_gates_integration.pytests/workspace/test_workspace_manager_comprehensive.pytests/agents/test_backend_worker_agent.pytests/lib/test_checkpoint_manager.pytests/e2e/seed-test-data.pytests/lib/test_quality_gate_tool.pytests/api/test_chat_api.py
tests/e2e/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Files:
tests/e2e/test_full_workflow.pytests/e2e/seed-test-data.py
codeframe/persistence/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Scope context items and agent data by (project_id, agent_id) tuple for multi-agent support
Files:
codeframe/persistence/migrations/migration_009_add_project_agents.pycodeframe/persistence/migrations/migration_007_sprint10_review_polish.py
codeframe/agents/worker_agent.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement quality gates with 4 stages (tests → type checking → coverage → review) before task completion
Files:
codeframe/agents/worker_agent.py
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Files:
tests/e2e/e2e-config.tstests/e2e/global-setup.ts
codeframe/ui/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/ui/**/*.py: Use FastAPI for all backend HTTP API endpoints
Use websockets for real-time Dashboard updates and multi-agent state synchronization
Files:
codeframe/ui/routers/session.pycodeframe/ui/routers/review.pycodeframe/ui/routers/blockers.pycodeframe/ui/routers/quality_gates.pycodeframe/ui/routers/checkpoints.py
codeframe/core/session_manager.py
📄 CodeRabbit inference engine (CLAUDE.md)
codeframe/core/session_manager.py: Store session state in .codeframe/session_state.json with automatic save on CLI exit and restore on startup
Set file permissions to 0o600 (owner-only) for session state files
Files:
codeframe/core/session_manager.py
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Run E2E tests before every release to catch regressions
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/lib/metrics_tracker.py : Record token usage with tracking of model_name, input_tokens, output_tokens, call_type, task_id, agent_id, and project_id
Applied to files:
codeframe/lib/metrics_tracker.pycodeframe/lib/sdk_hooks.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/lib/checkpoint_manager.py : Implement checkpoint system with Git commits, SQLite backups, and context snapshots in .codeframe/checkpoints/
Applied to files:
tests/e2e/test_full_workflow.pytests/lib/test_checkpoint_manager.pytests/e2e/seed-test-data.pycodeframe/ui/routers/checkpoints.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/persistence/**/*.py : Scope context items and agent data by (project_id, agent_id) tuple for multi-agent support
Applied to files:
codeframe/persistence/migrations/migration_009_add_project_agents.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/persistence/database.py : Track agent_id column in context_items table schema for multi-agent context isolation
Applied to files:
codeframe/persistence/migrations/migration_009_add_project_agents.pytests/lib/test_checkpoint_manager.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/agents/worker_agent.py : Implement quality gates with 4 stages (tests → type checking → coverage → review) before task completion
Applied to files:
codeframe/agents/worker_agent.pycodeframe/agents/review_worker_agent.pytestsprite_tests/TC012_Quality_Gates_Enforcement.pytests/lib/test_quality_gates.pycodeframe/lib/quality_gates.pytests/integration/test_quality_gates_integration.pycodeframe/ui/routers/quality_gates.pycodeframe/persistence/migrations/migration_007_sprint10_review_polish.pytests/e2e/seed-test-data.pytests/lib/test_quality_gate_tool.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/ui/server.py : Use uvicorn with FastAPI for backend server with auto-reload support and port validation
Applied to files:
tests/ui/test_deployment_mode.pytests/ui/test_project_api.pytests/api/test_blocker_resolution_api.pycodeframe/cli.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Applied to files:
tests/e2e/e2e-config.tstestsprite_tests/TC002_Project_Creation_Input_Validation_and_Error_Handling.pytests/e2e/global-setup.tstestsprite_tests/TC010_Code_Review_Panel_Updates_on_WebSocket_Events.pytestsprite_tests/TC004_Dashboard_Real_Time_Updates_and_Visualization.pytestsprite_tests/TC003_Discovery_QA_Completion_Workflow.pytestsprite_tests/TC006_Hierarchical_Task_Management_and_Dependency_Visualization.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Applied to files:
tests/e2e/e2e-config.tstests/e2e/global-setup.tstestsprite_tests/TC010_Code_Review_Panel_Updates_on_WebSocket_Events.pytestsprite_tests/TC011_Lint_Quality_Trend_Chart_Auto_Refresh_and_Visualization.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Applied to files:
tests/e2e/e2e-config.tstests/e2e/global-setup.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/e2e-config.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/**/*.py : Write tests using pytest with 100% async/await support for worker agent tests
Applied to files:
testsprite_tests/TC002_Project_Creation_Input_Validation_and_Error_Handling.pytestsprite_tests/TC013_Chat_Interface_Real_Time_Messaging_and_Markdown_Support.pytestsprite_tests/TC001_Project_Creation_Success.pytestsprite_tests/TC001_Project_Creation_with_Valid_Inputs.pytestsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.pytestsprite_tests/TC003_Multi_Agent_State_Synchronization_via_WebSocket.pytestsprite_tests/TC016_Context_Memory_Item_List_with_Pagination.pytestsprite_tests/TC012_Quality_Gates_Enforcement.pytestsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_Display_and_Resolution.pytestsprite_tests/TC010_Code_Review_Panel_Updates_on_WebSocket_Events.pytestsprite_tests/TC004_Dashboard_Real_Time_Updates_and_Visualization.pytestsprite_tests/TC007_Human_in_the_Loop_Blocker_Creation_and_Resolution.pytestsprite_tests/TC006_Multi_Agent_Concurrent_Execution_and_Status_Updates.pytestsprite_tests/TC003_Discovery_QA_Completion_Workflow.pytestsprite_tests/TC011_Lint_Quality_Trend_Chart_Auto_Refresh_and_Visualization.pytestsprite_tests/TC006_Hierarchical_Task_Management_Display.pytestsprite_tests/TC006_Hierarchical_Task_Management_and_Dependency_Visualization.pytestsprite_tests/TC011_API_Client_Response_and_Type_Safety_Validation.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/ui/**/*.py : Use websockets for real-time Dashboard updates and multi-agent state synchronization
Applied to files:
testsprite_tests/TC003_Multi_Agent_State_Synchronization_via_WebSocket.pytestsprite_tests/TC010_Code_Review_Panel_Updates_on_WebSocket_Events.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/**/*.py : Use Ruff for linting Python code targeting Python 3.11
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/**/*.py : Run ruff linter on Python code using 'ruff check .' command
Applied to files:
.pre-commit-config.yaml
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Applied to files:
tests/e2e/global-setup.tstests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/ui/server.py : Implement health check endpoint at GET /health for server readiness verification
Applied to files:
tests/integration/test_dashboard_access.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Let quality gates run automatically on task completion and only bypass for emergency hotfixes
Applied to files:
tests/integration/test_quality_gates_integration.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/core/session_manager.py : Store session state in .codeframe/session_state.json with automatic save on CLI exit and restore on startup
Applied to files:
codeframe/core/session_manager.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Create checkpoints before major refactors, risky changes, or at phase transitions
Applied to files:
tests/e2e/seed-test-data.py
🧬 Code graph analysis (21)
tests/e2e/test_full_workflow.py (1)
codeframe/agents/worker_agent.py (1)
WorkerAgent(7-493)
tests/e2e/e2e-config.ts (1)
ecosystem.staging.config.js (1)
path(1-1)
testsprite_tests/TC001_Project_Creation_with_Valid_Inputs.py (3)
testsprite_tests/TC001_Project_Creation_Success.py (1)
run_test(6-103)testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC003_Multi_Agent_State_Synchronization_via_WebSocket.py (1)
run_test(6-322)
codeframe/agents/backend_worker_agent.py (1)
codeframe/providers/sdk_client.py (1)
send_message(70-107)
testsprite_tests/TC016_Context_Memory_Item_List_with_Pagination.py (4)
testsprite_tests/TC001_Project_Creation_Success.py (1)
run_test(6-103)testsprite_tests/TC002_Project_Creation_Input_Validation_and_Error_Handling.py (1)
run_test(6-100)testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC004_PRD_Viewer_Rendering.py (1)
run_test(6-94)
codeframe/ui/routers/session.py (2)
codeframe/ui/routers/projects.py (1)
get_session_state(357-420)codeframe/ui/dependencies.py (1)
get_db(14-29)
tests/api/test_discovery_endpoints.py (1)
tests/api/conftest.py (1)
api_client(42-89)
tests/e2e/global-setup.ts (2)
tests/e2e/e2e-config.ts (1)
TEST_DB_PATH(8-8)ecosystem.staging.config.js (1)
path(1-1)
codeframe/ui/routers/blockers.py (2)
codeframe/persistence/database.py (2)
Database(23-3656)get_blocker(944-956)codeframe/ui/dependencies.py (1)
get_db(14-29)
testsprite_tests/TC004_Dashboard_Real_Time_Updates_and_Visualization.py (2)
testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC003_Multi_Agent_State_Synchronization_via_WebSocket.py (1)
run_test(6-322)
codeframe/lib/quality_gates.py (4)
codeframe/core/models.py (3)
Severity(72-79)Task(132-157)QualityGateFailure(749-755)codeframe/agents/worker_agent.py (1)
_create_quality_blocker(428-493)tests/lib/test_quality_gates.py (3)
task(56-79)task(467-488)task(901-923)web-ui/src/types/qualityGates.ts (1)
QualityGateFailure(27-32)
tests/integration/test_quality_gates_integration.py (2)
tests/lib/test_quality_gates.py (12)
quality_gates(82-84)quality_gates(491-493)quality_gates(926-928)db(32-37)db(443-448)db(877-882)project_id(47-53)project_id(458-464)project_id(892-898)project_root(40-44)project_root(451-455)project_root(885-889)codeframe/lib/quality_gates.py (1)
QualityGates(69-966)
tests/workspace/test_workspace_manager_comprehensive.py (2)
codeframe/workspace/manager.py (1)
create_workspace(26-71)codeframe/ui/models.py (1)
SourceType(11-17)
testsprite_tests/TC007_Human_in_the_Loop_Blocker_Creation_and_Resolution.py (3)
testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_and_Resolution.py (1)
run_test(6-106)testsprite_tests/TC007_Code_Review_Panel_Auto_Update_and_Details_Display.py (1)
run_test(6-334)
testsprite_tests/TC012_Real_Time_Chat_Interface_with_Lead_Agent.py (2)
testsprite_tests/TC001_Project_Creation_Success.py (1)
run_test(6-103)testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)
testsprite_tests/TC011_Lint_Quality_Trend_Chart_Auto_Refresh_and_Visualization.py (2)
testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC004_PRD_Viewer_Rendering.py (1)
run_test(6-94)
codeframe/lib/sdk_hooks.py (1)
tests/lib/test_sdk_hooks.py (2)
pre_hook(52-54)post_hook(58-60)
testsprite_tests/TC006_Hierarchical_Task_Management_Display.py (2)
testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC005_Human_in_the_Loop_Blocker_Creation_Display_and_Resolution.py (1)
run_test(6-362)
tests/e2e/seed-test-data.py (2)
ecosystem.staging.config.js (1)
path(1-1)codeframe/cli.py (1)
checkpoint(155-160)
codeframe/ui/routers/checkpoints.py (3)
codeframe/ui/models.py (1)
CheckpointCreateRequest(97-104)codeframe/persistence/database.py (2)
get_checkpoint(2844-2856)delete_checkpoint(3458-3466)codeframe/ui/shared.py (1)
broadcast(34-45)
tests/api/test_chat_api.py (3)
codeframe/persistence/database.py (1)
create_project(555-600)codeframe/ui/routers/projects.py (1)
create_project(57-195)tests/api/conftest.py (1)
api_client(42-89)
…icates - Remove 10+ duplicate identical form submission blocks - Add explicit validation after each form submission: * Test 1: Invalid name (leading hyphen) - assert form visible * Test 2: Name too short (<3 chars) - assert form visible * Test 3: Description too short (<10 chars) - assert form visible * Test 4: Valid input - wait for URL change or success indicator * Test 5: Verify session state/dashboard loaded - Each test case clears/modifies input before proceeding - Add meaningful assertions with clear error messages - Replace generic assertion with actual UI element checks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/global-setup.ts (1)
53-71: Consider extracting inline Python to a separate script.Lines 58-59 embed Python code directly in a command string, making it harder to test and maintain. Consider creating
tests/e2e/init-test-database.pyto match the pattern used inseedDatabaseDirectly()(line 86).Example refactor:
// Create tests/e2e/init-test-database.py with: // #!/usr/bin/env python3 // import sys // from codeframe.persistence.database import Database // db = Database(sys.argv[1]) // db.initialize() const scriptPath = path.join(__dirname, 'init-test-database.py'); const result = spawnSync('uv', ['run', 'python3', scriptPath, TEST_DB_PATH], { cwd: projectRoot, stdio: 'inherit', encoding: 'utf-8', });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/global-setup.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Files:
tests/e2e/global-setup.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Run E2E tests before every release to catch regressions
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Applied to files:
tests/e2e/global-setup.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Applied to files:
tests/e2e/global-setup.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Applied to files:
tests/e2e/global-setup.ts
🧬 Code graph analysis (1)
tests/e2e/global-setup.ts (1)
tests/e2e/e2e-config.ts (2)
TEST_DB_PATH(8-8)BACKEND_URL(11-11)
🔇 Additional comments (4)
tests/e2e/global-setup.ts (4)
5-9: LGTM! Import cleanup aligns with architectural changes.The removal of
Pagetype (previously used by deleted API-based seed functions) and addition ofspawnSyncfor direct database operations correctly support the shift to file-system-based test data management.
142-148: Excellent: Project reuse prevents unnecessary recreation.The logic to reuse an existing
e2e-test-project(or fall back to any existing project) reduces setup time and avoids workspace conflicts. The retry logic (lines 160-176) provides good resilience when creation fails.
119-123: Setup flow correctly ordered.Calling
cleanupTestEnvironment()beforeinitializeTestDatabase()ensures a clean workspace state, though verify that database reuse (per comment in lines 14-16) doesn't cause test pollution.
17-30: Remove the incorrect comment about server startup order; database reuse is intentional and managed via test data seeding.The claim that the "backend server starts before globalSetup" is incorrect. Playwright's architecture runs
globalSetupbefore startingwebServer(with health check polling at line 60). The database is intentionally reused locally (viareuseExistingServer: !process.env.CI), but this is safe becauseseed-test-data.pyexplicitly manages test data isolation. SinceDatabase.initialize()usesCREATE TABLE IF NOT EXISTS, it doesn't drop existing tables—it only ensures the schema exists. This is the intended behavior; the database persists across runs locally to matchreuseExistingServer: true.The cleanup strategy is correct: remove only the workspaces directory (to avoid "workspace already exists" errors) while letting the database persist. If test pollution becomes an issue, consider adding explicit data cleanup between test suites in
seed-test-data.pyor using test-specific project IDs, but the current approach aligns with Playwright best practices for server reuse.
- Pattern 1: Now captures optional trailing fields after project_type
{"project_name":"foo","project_type":"python","extra":"field"}
→ {"name":"foo","description":"Test project","extra":"field"}
- Pattern 4: Now strips project_type from rest instead of skipping
{"project_name":"foo","other":"bar","project_type":"python"}
→ {"name":"foo","description":"Test project","other":"bar"}
- Pattern 5: Now handles extra fields after empty project_name
{"project_name":"","project_type":"python","extra":"field"}
→ {"name":"","description":"Test project","extra":"field"}
Change seedDatabaseDirectly() to use 'uv run python' instead of 'python3' directly, matching the approach used in initializeTestDatabase(). This ensures the seed script runs in the project's virtual environment with all dependencies available, avoiding import/dependency mismatches.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py (1)
48-50: Validation error locators are defined but never used.Three validation error locators are defined (
validation_error,min_chars_error,min_desc_error) but are never referenced in the test logic. The test currently only verifies that the form remains visible after invalid submissions but doesn't confirm that appropriate error messages are displayed to the user. This weakens test coverage - the form could be broken (not showing validation messages) and the test would still pass.Add explicit validation error checks after each negative test submission:
print("Test 1: Invalid project name with leading hyphen") await project_name_input.fill("-invalidname") await project_desc_input.fill("Valid description for testing session lifecycle.") await submit_button.click() # Wait and check for validation error OR navigation await page.wait_for_timeout(1000) + +# Verify validation error message is displayed +await expect(validation_error).to_be_visible(timeout=2000) current_url = page.urlApply similar checks for Test Cases 2 and 3 using
min_chars_errorandmin_desc_errorrespectively.
🧹 Nitpick comments (2)
testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py (2)
65-71: Simplify the URL validation logic.The URL parsing logic
"/" == current_url.split("3000")[-1].rstrip("/")is fragile and hard to read. It assumes the port is always "3000" and could break with URL changes.Simplify using a more direct URL check:
-if "localhost:3000" in current_url and "/" == current_url.split("3000")[-1].rstrip("/"): +# Check we're still on the home/root page +if current_url.rstrip("/").endswith(("localhost:3000", "localhost:3000/")): # Check that we're still on the form (validation prevented submission) is_form_visible = await submit_button.is_visible() assert is_form_visible, "Form should still be visible after invalid input" print(" ✓ Validation prevented submission with invalid name") else: raise AssertionError("Expected to remain on form page due to validation error")Or even simpler, just check the form visibility without the URL check, since that's the key indicator:
-if "localhost:3000" in current_url and "/" == current_url.split("3000")[-1].rstrip("/"): - # Check that we're still on the form (validation prevented submission) - is_form_visible = await submit_button.is_visible() - assert is_form_visible, "Form should still be visible after invalid input" - print(" ✓ Validation prevented submission with invalid name") -else: - raise AssertionError("Expected to remain on form page due to validation error") +# Check that we're still on the form (validation prevented submission) +is_form_visible = await submit_button.is_visible() +assert is_form_visible, "Form should still be visible after invalid input" +print(" ✓ Validation prevented submission with invalid name")
175-175: Consider removing or documenting the 2-second sleep.The
await asyncio.sleep(2)before cleanup appears arbitrary. If it's for manual observation during test development, consider removing it to speed up test execution. If it's necessary for the test to complete properly, add a comment explaining why.- await asyncio.sleep(2) + # Brief pause for observation (remove in CI) + # await asyncio.sleep(2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Run E2E tests before every release to catch regressions
🧬 Code graph analysis (1)
testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py (3)
testsprite_tests/TC002_Project_Creation_Input_Validation_and_Error_Handling.py (1)
run_test(6-100)testsprite_tests/TC003_Discovery_QA_Completion_Workflow.py (1)
run_test(6-87)testsprite_tests/TC004_PRD_Viewer_Rendering.py (1)
run_test(6-94)
🔇 Additional comments (1)
testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py (1)
101-133: Positive test case handles multiple success scenarios appropriately.The test correctly handles uncertainty about whether project creation navigates to a new page or shows a success indicator on the same page. The nested try-except blocks make the logic harder to follow, but they properly verify success through multiple possible indicators, which addresses the validation concerns from previous reviews.
Consider extracting this into a helper function for reusability:
async def verify_project_creation_success(page, submit_button, timeout=10000): """Verify project was created by checking for navigation or success indicators.""" try: await page.wait_for_url("**/projects/**", timeout=timeout) return True, "navigation" except async_api.Error: try: success_indicator = page.locator("text=Project created").or_( page.locator("text=Discovery") ).or_(page.locator("[data-testid='project-dashboard']")) await success_indicator.wait_for(state="visible", timeout=5000) return True, "indicator" except async_api.Error: if await submit_button.is_visible(): return False, "form_visible" return False, "unknown"
| # Navigate to your target URL and wait until the network request is committed | ||
|
|
||
| # Navigate to the application | ||
| await page.goto("http://localhost:3000", wait_until="commit", timeout=10000) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace hardcoded URL with a configuration constant.
The URL "http://localhost:3000" is hardcoded here and at line 142. Based on the PR context, the Playwright configuration defines a FRONTEND_URL constant. Use that constant instead to avoid maintenance issues and ensure consistency with the test environment configuration.
Apply this pattern to use a configuration constant:
+# At the top of the file, after imports
+FRONTEND_URL = "http://localhost:3000" # Or import from config
+
# Navigate to the application
-await page.goto("http://localhost:3000", wait_until="commit", timeout=10000)
+await page.goto(FRONTEND_URL, wait_until="commit", timeout=10000)And at line 142:
-assert "localhost:3000" in current_url, f"Unexpected URL: {current_url}"
+assert FRONTEND_URL in current_url, f"Unexpected URL: {current_url}"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py at
line 34 (and also at line 142), replace the hardcoded "http://localhost:3000"
with the Playwright configuration constant FRONTEND_URL; import or reference
FRONTEND_URL from the test/config module where Playwright exports it, and keep
the existing arguments (wait_until="commit", timeout=10000) unchanged so both
navigations use FRONTEND_URL consistently.
| # ======================================== | ||
| # Test Case 5: Verify session state persistence | ||
| # ======================================== | ||
| if project_created: | ||
| print("Test 5: Verify session state/navigation") | ||
| # After project creation, verify we're on a project-related page | ||
| current_url = page.url | ||
| assert "localhost:3000" in current_url, f"Unexpected URL: {current_url}" | ||
|
|
||
| # Look for session-related content or project dashboard elements | ||
| try: | ||
| # Wait for any dashboard or project content to load | ||
| await page.wait_for_load_state("networkidle", timeout=10000) | ||
|
|
||
| # Check for common dashboard elements | ||
| dashboard_content = page.locator("main").or_( | ||
| page.locator("[role='main']") | ||
| ).or_( | ||
| page.locator(".dashboard") | ||
| ) | ||
| await dashboard_content.wait_for(state="visible", timeout=5000) | ||
| print(" ✓ Dashboard/project content loaded") | ||
| except async_api.Error: | ||
| print(" ⚠ Could not verify dashboard content (may still be loading)") | ||
|
|
||
| # ======================================== | ||
| # Final Assertion | ||
| # ======================================== | ||
| print("\n--- Final Verification ---") | ||
| # The test passes if we successfully created a project and navigated away from the form | ||
| if project_created: | ||
| print("✅ Session lifecycle test completed successfully") | ||
| print(" - Validation errors properly blocked invalid submissions") | ||
| print(" - Valid project was created") | ||
| print(" - Navigation/state change confirmed") | ||
| else: | ||
| raise AssertionError( | ||
| "Test case failed: Could not create project and verify session state" | ||
| ) |
There was a problem hiding this comment.
Test name promises "Resumption" but doesn't test it.
The test is named TC010_Session_Lifecycle_Persistence_and_Resumption.py, but it only verifies project creation and navigation - not actual session resumption. True resumption testing would involve:
- Creating a project and noting the session state
- Closing the browser or clearing context
- Reopening and verifying the session is restored/resumed
Additionally, line 158 prints a warning when dashboard content can't be verified but doesn't fail the test, which weakens the session state assertions.
Either:
- Rename the test to accurately reflect what it tests (e.g.,
TC010_Project_Creation_Validation_and_Navigation.py), OR - Add actual resumption testing:
await dashboard_content.wait_for(state="visible", timeout=5000)
print(" ✓ Dashboard/project content loaded")
+
+ # Store current state for resumption test
+ project_url = page.url
+
+ # Close and reopen browser to test session resumption
+ await context.close()
+ await browser.close()
+
+ # Reopen browser and context
+ browser = await pw.chromium.launch(headless=True, args=[...])
+ context = await browser.new_context()
+ page = await context.new_page()
+
+ # Navigate back and verify session is restored
+ await page.goto(project_url)
+ await dashboard_content.wait_for(state="visible", timeout=5000)
+ print(" ✓ Session successfully resumed")
except async_api.Error:
- print(" ⚠ Could not verify dashboard content (may still be loading)")
+ raise AssertionError("Could not verify dashboard content or session resumption")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In testsprite_tests/TC010_Session_Lifecycle_Persistence_and_Resumption.py around
lines 135–173, the test only checks project creation/navigation but does not
exercise session "resumption" and currently treats missing dashboard detection
as a non-fatal warning; update the test to actually validate session persistence
by (1) after creating the project capture the browser storage state or session
cookies (localStorage/sessionStorage or auth cookie), (2) close the page/browser
context, (3) create a new context/page and restore the captured storage or set
the cookie before navigation, (4) navigate to the app and assert the
dashboard/project page is visible (fail the test if not), and also replace the
non-failing print in the except block with an assertion or raise so inability to
verify dashboard content fails the test; alternatively, if you prefer not to add
resumption steps, rename the file and test to reflect only
creation/validation/navigation (e.g.,
TC010_Project_Creation_Validation_and_Navigation.py) and keep the existing
behavior.
Dashboard.tsx references the 'checkpoints' tab but the type was missing from the DashboardTab union, causing TypeScript compilation errors in CI.
Code Review: Checkpoint project_id FixThanks for this comprehensive fix! This PR addresses the root cause of issue #89 by synchronizing database paths and adding proper checkpoint seeding. Strengths
Issues and Concerns1. Critical: Database Path Inconsistency (High Priority)Location: tests/e2e/playwright.config.ts:90 The backend server command hardcodes the database path instead of using TEST_DB_PATH. This defeats the purpose of centralizing TEST_DB_PATH in e2e-config.ts. Recommendation: Use TEST_DB_PATH in the command string instead of recreating the path. 2. Potential Race Condition (Medium Priority)Location: tests/e2e/global-setup.ts:125-136 Database initialization happens after cleanup but the backend server starts before globalSetup. If the backend server starts and creates tables before initializeTestDatabase(), conflicts may occur. Recommendation: Add a comment explaining why this is safe OR move initialization earlier. 3. Checkpoint File Path Logic (Medium Priority)Location: tests/e2e/seed-test-data.py:985-988 The code assumes db_dir is always .codeframe, but db_path could be an absolute path causing incorrect checkpoint paths. Recommendation: Use explicit project root detection based on the script location. 4. Missing Test Coverage for Checkpoint Files (Low Priority)The seeding script creates checkpoint files but there is no test verifying these files exist and are valid. 5. Hardcoded Timestamps (Low Priority)Task timestamps use relative time deltas making test data non-deterministic. Recommendation: Use fixed timestamps for reproducibility. Security Review
Recommendations SummaryMust Fix (Blockers):
Should Fix (High Priority): Nice to Have: Approval StatusConditional Approval: This PR is well-designed and solves the root cause of #89. Please address the Critical issue before merging. Overall Assessment: 8.5/10 - Excellent architectural fix with minor implementation issues to address. |
Code Review SummaryThis PR addresses issue #89 by fixing the checkpoint project_id mismatch in E2E tests. The solution properly synchronizes database paths and adds checkpoint seeding. Overall, the approach is solid, but I've identified several areas for improvement. Strengths
Issues & Recommendations1. Security: Command Injection Risk (Medium Severity)Location: tests/e2e/global-setup.ts:58-59 The pythonCode variable is passed through sys.argv which could be problematic if TEST_DB_PATH contains special characters. Recommendation: Use environment variables or a temporary Python script file for better safety. 2. Path Handling: Platform Compatibility (Low-Medium Severity)Location: tests/e2e/seed-test-data.py:986-988 The checkpoint paths in the database use forward slashes (.codeframe/checkpoints/...), which may not work correctly on Windows. Recommendation: Normalize paths before joining using os.sep. 3. Database Backup: Incomplete Checkpoint Files (Medium Severity)Location: tests/e2e/seed-test-data.py:990-1020 The checkpoint backup databases only contain metadata, not the actual project state (tasks, agents, etc.). This makes checkpoint restoration tests potentially misleading. Recommendation: Copy the entire test database to create realistic checkpoint backups instead of creating minimal metadata-only files. 4. Error Handling: Silent Failures (Low-Medium Severity)Location: tests/e2e/seed-test-data.py:1019-1020, 1029-1030 Checkpoint file creation failures are logged but don't fail the seeding process. This could lead to tests passing with incomplete test data. Recommendation: Make file creation failures fatal by re-raising exceptions. 5. Testing Gap: No Validation (Medium Severity)Location: tests/e2e/global-setup.ts:197 After seeding, there's no validation that the checkpoint data was actually created correctly. Recommendation: Add post-seeding validation by querying the checkpoints API to verify the expected checkpoints exist. 6. Performance: Unnecessary Database Connections (Low Severity)Location: tests/e2e/seed-test-data.py:994-1017 Each checkpoint backup creates a new SQLite connection. Recommendation: Use context managers for automatic connection cleanup. Test CoverageObservation: The PR description's test plan shows:
Recommendation: Complete the E2E test suite run before merging and add results to PR description. Security Considerations
DocumentationMissing:
Recommendation: Add comments to e2e-config.ts explaining the rationale behind the fixed TEST_DB_PATH. Best Practices ComplianceBased on CLAUDE.md:
VerdictStatus: Approve with minor changes recommended Priority Fixes Before Merge:
Can Be Addressed Post-Merge:
Overall Assessment: This is a well-thought-out fix that addresses the root cause effectively. The centralized configuration pattern is excellent and should prevent similar issues in the future. With the priority fixes, this will be production-ready. Great work! |
1. Critical: Use TEST_DB_PATH constant in playwright.config.ts instead of hardcoded path - centralizes database path configuration 2. Medium: Add documentation explaining Playwright webServer ordering (backend starts before globalSetup) and why it's safe 3. Medium: Use E2E_TEST_ROOT (derived from script location) for checkpoint file paths instead of deriving from db_path 4. Low: Add verify_checkpoint_files() to validate checkpoint files after seeding (checks SQLite validity and JSON parsing) 5. Low: Use fixed timestamp (2025-01-15 10:00:00) for reproducible test data instead of datetime.now()
Code Review: Checkpoint project_id Fix + Major RefactoringSummary: This PR successfully fixes issue #89 (checkpoint project_id mismatch) while delivering a substantial refactoring that removes 6,096 lines and adds 5,810 lines across 127 files. The core fix is sound, but the massive scope introduces risks. Strengths1. Root Cause Fix is CorrectThe E2E test seeding changes properly address the database path synchronization issue:
This eliminates the mismatch where the backend used a production database while tests seeded to a different location. 2. Checkpoint Seeding is RobustThe Python seeding script demonstrates excellent practices:
3. Security Best Practicesglobal-setup.ts properly prevents command injection by using argument arrays instead of string interpolation in spawnSync(). Concerns1. PR Scope is Excessive (High Risk)127 files changed, 11,906 lines modified far exceeds the scope of fixing checkpoint project_id. Evidence:
Risk: Large refactorings bundled with bug fixes make it:
Recommendation: Consider splitting into multiple PRs (minimal fix, then refactorings). 2. Missing Test VerificationThe PR body has an incomplete test plan - the third checkbox (full E2E test suite) is unchecked, yet this is the primary acceptance criterion from issue #89. Questions:
3. Checkpoint File Path Resolution Inconsistencyseed-test-data.py:990-994 uses E2E_TEST_ROOT for checkpoint file paths (derived from script location), but the database uses a different root derived from the db_path parameter. Risk: If db_path is outside tests/e2e/ (e.g., /tmp/test.db), checkpoint files will be written to E2E_TEST_ROOT but the database will reference paths relative to /tmp/. Recommendation: Use consistent base directory derived from db_path parameter. 4. Checkpoint Verification Only Checks 2/3 Checkpointsverify_checkpoint_files() at line 1066 only validates checkpoint-001 and checkpoint-002, but the seeding creates 3 checkpoints (lines 926-965). Fix: Add checkpoint-003 files to expected_files list. Test CoverageE2E Test Seeding - Excellent:
Missing Unit Tests: Recommendation: Add tests for global-setup.ts functions and seed-test-data.py verification logic. Security
Actionable RecommendationsMust-Fix Before Merge:
Should-Fix Before Merge: Nice-to-Have: Summary Score
Overall: 4/5 - Strong implementation with scope concerns VerdictConditionally approve pending:
The core checkpoint project_id fix is sound and addresses issue #89 correctly. However, bundling a massive refactoring with a critical bug fix creates review burden and increases regression risk. Consider splitting to improve maintainability. Great work on the security practices and comprehensive test data seeding! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/fix_api_schema.py (1)
41-59: Pattern 4 correctly removes project_type and handles empty rest.The logic properly strips
project_typefrom the remaining fields and falls back to a base object when no fields remain. The twore.subpatterns handle different positions ofproject_typewithin the field list.For additional robustness, consider using a single regex with alternation to handle both patterns in one pass:
- rest = re.sub(r',?\s*"project_type":\s*"[^"]+"', '', rest) - rest = re.sub(r'"project_type":\s*"[^"]+",?\s*', '', rest) + rest = re.sub(r'(?:,?\s*"project_type":\s*"[^"]+")|(?:"project_type":\s*"[^"]+",?\s*)', '', rest)However, the current approach is clear and handles the common cases effectively.
tests/e2e/seed-test-data.py (1)
990-994: Path construction approach is sound.Using
E2E_TEST_ROOTto construct absolute paths is more reliable than deriving fromdb_path. The comment clearly explains the rationale.Consider verifying that the constructed absolute paths actually resolve to the expected location relative to the database. For example, if the database is at
tests/e2e/.codeframe/state.dband checkpoint files are attests/e2e/.codeframe/checkpoints/..., then the relative paths stored in the database (.codeframe/checkpoints/...) might not resolve correctly when accessed from the project root.However, if the backend/tests always work from the E2E test root when resolving these paths, the current approach is correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/fix_api_schema.py(2 hunks)tests/e2e/global-setup.ts(4 hunks)tests/e2e/seed-test-data.py(14 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Files:
tests/e2e/global-setup.ts
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Write tests using pytest with 100% async/await support for worker agent tests
Files:
tests/e2e/seed-test-data.py
tests/e2e/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Files:
tests/e2e/seed-test-data.py
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Run E2E tests before every release to catch regressions
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.py : Implement database seeding with INSERT OR REPLACE to avoid UNIQUE constraint conflicts in E2E tests
Applied to files:
tests/e2e/global-setup.tstests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : Use npm and TypeScript for frontend testing with Playwright for E2E browser automation
Applied to files:
tests/e2e/global-setup.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Use Playwright's webServer config with health check polling instead of manual server startup in E2E tests
Applied to files:
tests/e2e/global-setup.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to tests/e2e/playwright.config.ts : Reuse existing servers in E2E tests when reuseExistingServer is enabled (default locally, disabled in CI)
Applied to files:
tests/e2e/global-setup.ts
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/lib/checkpoint_manager.py : Implement checkpoint system with Git commits, SQLite backups, and context snapshots in .codeframe/checkpoints/
Applied to files:
tests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Create checkpoints before major refactors, risky changes, or at phase transitions
Applied to files:
tests/e2e/seed-test-data.py
📚 Learning: 2025-12-11T17:15:19.102Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-11T17:15:19.102Z
Learning: Applies to codeframe/agents/worker_agent.py : Implement quality gates with 4 stages (tests → type checking → coverage → review) before task completion
Applied to files:
tests/e2e/seed-test-data.py
🧬 Code graph analysis (1)
tests/e2e/global-setup.ts (1)
tests/e2e/e2e-config.ts (2)
TEST_DB_PATH(8-8)BACKEND_URL(11-11)
⏰ 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 (12)
scripts/fix_api_schema.py (3)
13-25: LGTM! Pattern 1 now correctly handles extra fields.The enhanced Pattern 1 addresses the previous review's concern by capturing optional extra fields after
project_typeand preserving them in the transformed output. The regex pattern and replacement logic correctly handle both cases (with and without extra fields).
62-73: LGTM! Pattern 5 mirrors Pattern 1's approach for empty project_name.The implementation correctly handles the edge case of empty
project_namewith optional extra fields, maintaining consistency with Pattern 1's design.
76-83: LGTM! String replacements align with the new schema.The terminology updates ensure consistency between the code, comments, and test assertions.
tests/e2e/seed-test-data.py (3)
6-31: LGTM! Well-structured constants and helper function.The new table name constants and
table_exists()helper improve code maintainability and prevent typos. UsingE2E_TEST_ROOTderived from the script location ensures reliable path resolution for checkpoint files.
876-1044: Excellent! Checkpoint seeding now creates actual files.This implementation addresses the critical issue from the previous review by creating both database records AND the actual checkpoint files (SQLite backups and JSON context snapshots). The approach is thorough:
- Creates checkpoint directory structure
- Generates valid SQLite backup files with metadata
- Writes JSON context snapshots
- Uses
INSERT OR REPLACEper coding guidelines- Handles filesystem errors gracefully
Based on learnings, checkpoint file creation is essential for test correctness as multiple test files assert file existence.
996-1026: LGTM! SQLite backup file creation is correct.The implementation creates valid SQLite database files with a
checkpoint_infotable containing metadata. UsingINSERT OR REPLACEensures idempotency, and error handling is appropriate for a seeding script.tests/e2e/global-setup.ts (6)
17-30: LGTM! Cleanup strategy is safe and well-reasoned.The function correctly limits cleanup to the workspaces directory to avoid database locking issues, with clear documentation explaining why the database is not deleted.
43-72: LGTM! Database initialization is secure and reliable.The function properly initializes the database schema using the backend's
Database.initialize()method. UsingspawnSyncwith an argument array prevents command injection, and error handling is appropriate.
78-117: LGTM! Seeding now uses consistent Python execution.This addresses the critical issue from the previous review by using
uv run pythonconsistently withinitializeTestDatabase(). The implementation is secure (uses argument arrays) and includes appropriate error handling with an optional graceful degradation mode.
119-133: LGTM! Setup flow correctly handles Playwright's execution order.The initialization sequence properly accounts for Playwright's webServer starting before globalSetup runs. The detailed comment clearly explains the constraint and why the approach is safe.
143-196: LGTM! Robust project creation with appropriate fallbacks.The logic properly handles multiple scenarios (existing project, need to create, creation fails) with clear fallback paths. The retry logic specifically handles "workspace already exists" errors that can occur from previous runs, preventing flaky test setup.
5-5: LGTM! Removed unused import.The
Pageimport removal is correct—verification confirmsPageis not used anywhere in the file, and the old API-based seed functions (seedAgents,seedTasks,seedTokenUsage,seedCheckpoints,seedReviews) have already been removed. The file properly uses TypeScript and Playwright as required.
Test files were using hardcoded port 8000 while actual API clients default to port 8080. This caused 14 test failures in checkpoints and metrics API tests.
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
Code Review: PR #92 - Checkpoint project_id FixThis PR addresses checkpoint project_id mismatch in E2E tests by synchronizing database paths and adding direct checkpoint seeding. The core changes are sound and well-structured, with a few concerns to address. Strengths1. Root Cause AnalysisThe PR correctly identifies the issue:
2. Centralized Configuration (e2e-config.ts)Excellent pattern! Centralizing TEST_DB_PATH, BACKEND_URL, and FRONTEND_URL in one file eliminates path mismatches and ensures consistency with Playwright config. 3. Robust Checkpoint SeedingThe Python seeding script is comprehensive:
4. Defensive Error HandlingGood use of INSERT OR REPLACE to avoid UNIQUE constraint warnings, and try-catch blocks for file operations. Concerns and IssuesCRITICAL: Path Calculation LogicLocation: seed-test-data.py:990-994 Issue: The code joins E2E_TEST_ROOT with checkpoint paths, creating files at tests/e2e/.codeframe/checkpoints/..., but the database stores paths as .codeframe/checkpoints/... which are relative to PROJECT ROOT, not E2E_TEST_ROOT. Mismatch:
Recommendation: Use PROJECT_ROOT instead of E2E_TEST_ROOT: MEDIUM: Timestamp InconsistencyLocation: seed-test-data.py:44 vs 452 Line 44 sets fixed timestamp: now = datetime(2025, 1, 15, 10, 0, 0) Issue: Token usage records lose reproducibility. Recommendation: Remove line 452 and use the fixed timestamp throughout. MEDIUM: SDK Hooks Error HandlingEnsure defensive error handling for SDK hooks:
LOW: Verification Failure HandlingThe seeding script verifies checkpoint files but does not fail if verification fails. Consider adding E2E_STRICT_MODE flag for strict verification when checkpoints are critical. Code QualityGood:
Could Improve:
Security: No concerns - parameterized queries, no secrets Performance: Efficient - uses INSERT OR REPLACE, single transaction TestingTest Plan Status:
CI Status:
Action Required: Wait for E2E tests to complete before merging. Impact AnalysisFiles Changed: 96 files (5812 additions, 6098 deletions) - very large PR Breaking Changes: None (test infrastructure only) Documentation: Missing tests/e2e/README.md updates for new architecture RecommendationsBefore Merge (High Priority):
Nice to Have: VerdictStatus: Changes Requested Reason: Checkpoint file path bug will likely cause E2E test failures when restoring checkpoints (files not found at expected paths). Effort: Low (5-10 minutes) After fixes: Excellent improvement to E2E test reliability! Great work on the thorough root cause analysis! The centralized configuration pattern is exemplary. Just need to fix the path calculation before merging. |
1 participant
Summary
This PR completes the fix for #89 by addressing the root causes of checkpoint
project_idmismatch in E2E tests:findDatabasePath()with fixedTEST_DB_PATHthat matches Playwright configinitializeTestDatabase()to create directory and schema before seedingseed-test-data.py(3 checkpoints with correct project_id)Root Cause
The original issue (#89) was caused by:
Changes
tests/e2e/global-setup.tsTEST_DB_PATHconstant matching Playwright config (tests/e2e/.codeframe/state.db)initializeTestDatabase()functionseedDatabaseDirectly()to use fixed pathtests/e2e/seed-test-data.pyproject_idparameterTest plan
project_id=1GET /api/projects/1/checkpointsreturns all 3 checkpointscd tests/e2e && npx playwright testRelated
Fixes #89
Unblocks #85 (CheckpointList visibility)
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.