fix(e2e): resolve checkpoint UI test timeouts with race condition fix - #260
Conversation
Root cause: Tests were waiting for /checkpoints API response AFTER clicking the checkpoint tab, but the API call fires immediately on tab click, causing a race condition where the response was already received before the listener was set up. Fixes: 1. seed-test-data.py: Ensure Project 1 has user_id=1 for authorization - Added explicit ownership verification after test user creation - Added checkpoint record verification with file existence checks 2. test_checkpoint_ui.spec.ts: Fixed race condition - Set up API response listener BEFORE clicking checkpoint tab - Added comprehensive error monitoring and logging - Added API health check test that runs before main suite 3. checkpoints.ts: Enhanced error handling - Added CheckpointApiError class with status code tracking - Added specific logging for auth/authz errors (401/403) 4. CheckpointList.tsx: Improved testability - Added data-testid for loading and error states - Added retry button for error recovery These changes ensure that: - The checkpoint API is verified accessible before UI tests run - Race conditions are prevented by proper listener ordering - Error states are clearly identified in test output - Project authorization is guaranteed through seed script
WalkthroughThis PR adds data-integrity checks to the E2E seed script, strengthens E2E checkpoint tests with API health checks and error monitoring, introduces a typed CheckpointApiError and improved error handling in the checkpoints client, and adds test hooks (data-testids and retry) to the CheckpointList UI. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test (Playwright)
participant Auth as Auth Service
participant Backend as Backend API (/checkpoints)
participant DB as Database
participant FS as Filesystem (checkpoint files)
Test->>Auth: request auth token (getAuthToken)
Auth-->>Test: returns token
Test->>Backend: GET /checkpoints (with token)
Backend->>DB: query checkpoints for Project 1
DB-->>Backend: checkpoint records
Backend->>FS: verify checkpoint DB/context files (seed verification)
FS-->>Backend: file existence/status
Backend-->>Test: 200 + checkpoints array (or error)
Test->>UI: open CheckpointList and wait for /checkpoints response
UI-->>Test: render loading → list or error (uses data-testid hooks)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Fix E2E checkpoint UI test timeouts by making
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
web-ui/src/components/checkpoints/CheckpointList.tsx (1)
300-310: Error state with retry functionality.The error block now includes a
data-testidfor testability and a Retry button for error recovery. This aligns well with the enhanced error handling incheckpoints.ts.Consider adding an accessible label to the Retry button for screen readers:
♿ Accessibility suggestion
<button onClick={loadCheckpoints} className="mt-2 text-sm text-primary hover:underline" + aria-label="Retry loading checkpoints" > Retry </button>tests/e2e/test_checkpoint_ui.spec.ts (1)
69-70: Consider using a more specific type.The
checkpointApiResponsePromisevariable could use Playwright'sResponsetype instead ofanyfor better type safety.♻️ Type improvement
+import { test, expect, Response } from '@playwright/test'; ... - let checkpointApiResponsePromise: Promise<any> | null = null; + let checkpointApiResponsePromise: Promise<Response> | null = null;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/e2e/seed-test-data.pytests/e2e/test_checkpoint_ui.spec.tsweb-ui/src/api/checkpoints.tsweb-ui/src/components/checkpoints/CheckpointList.tsx
🧰 Additional context used
📓 Path-based instructions (4)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development
Use React 18 and Next.js 14 for frontend development
Use Tailwind CSS for styling with Nova design system template
Use shadcn/ui components from @/components/ui/ for UI elements
Use Hugeicons (@hugeicons/react) for all icons, never mix with lucide-react
Use Nova color palette variables (bg-card, text-foreground, etc.) instead of hardcoded color values
Use cn() utility for conditional CSS classes in React components
Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
Include auth token as query parameter in WebSocket connections (?token=TOKEN)
Store auth tokens in localStorage with key 'auth_token' and include token in API requests via Authorization header
Files:
web-ui/src/api/checkpoints.tsweb-ui/src/components/checkpoints/CheckpointList.tsx
tests/**/*.{ts,tsx,test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use test.skip() inside test logic; skip at describe level or use separate test projects for different states
Files:
tests/e2e/test_checkpoint_ui.spec.ts
tests/e2e/**/*.{ts,test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/e2e/**/*.{ts,test.ts}: Use loginUser() helper from tests/e2e/test-utils.ts for authentication in E2E tests
Assert UI elements existence with expect(element).toBeVisible() - fail if missing, never silently pass
Use TEST_PROJECT_IDS.PLANNING for tests requiring pre-seeded planning phase tasks
Use TEST_PROJECT_IDS.ACTIVE for tests requiring pre-seeded active phase with agents
Files:
tests/e2e/test_checkpoint_ui.spec.ts
web-ui/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.{ts,tsx}: Use React Context with useReducer for centralized state management in Dashboard
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Files:
web-ui/src/components/checkpoints/CheckpointList.tsx
🧠 Learnings (8)
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Applied to files:
web-ui/src/api/checkpoints.tsweb-ui/src/components/checkpoints/CheckpointList.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
Applied to files:
web-ui/src/api/checkpoints.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.ACTIVE for tests requiring pre-seeded active phase with agents
Applied to files:
tests/e2e/test_checkpoint_ui.spec.tstests/e2e/seed-test-data.py
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use loginUser() helper from tests/e2e/test-utils.ts for authentication in E2E tests
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.PLANNING for tests requiring pre-seeded planning phase tasks
Applied to files:
tests/e2e/test_checkpoint_ui.spec.tstests/e2e/seed-test-data.py
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Assert UI elements existence with expect(element).toBeVisible() - fail if missing, never silently pass
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/**/*.{ts,tsx,test.ts} : Never use test.skip() inside test logic; skip at describe level or use separate test projects for different states
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
🧬 Code graph analysis (3)
web-ui/src/api/checkpoints.ts (3)
web-ui/src/api/context.ts (1)
listCheckpoints(103-110)web-ui/src/types/checkpoints.ts (1)
Checkpoint(17-28)web-ui/src/lib/api-client.ts (1)
authFetch(106-146)
tests/e2e/test_checkpoint_ui.spec.ts (2)
tests/e2e/e2e-config.ts (2)
FRONTEND_URL(14-14)BACKEND_URL(11-11)tests/e2e/test-utils.ts (3)
loginUser(483-500)getAuthToken(560-562)setupErrorMonitoring(64-104)
tests/e2e/seed-test-data.py (1)
tests/test_review_api.py (1)
project_id(95-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: opencode-review
🔇 Additional comments (10)
web-ui/src/api/checkpoints.ts (2)
16-28: Well-structured custom error class.The
CheckpointApiErrorclass properly extendsError, sets thenameproperty for proper error identification, and includes useful debugging properties (statusCode,endpoint).
38-63: Error handling provides good debugging context.The try/catch wrapper with status code extraction and contextual logging will help diagnose authentication and authorization issues during tests. The status code extraction relies on the error message format from
authFetch("Request failed: ${status}"), which is a reasonable coupling within the same codebase.One consideration: if
authFetchthrows an error without following this format (e.g., network errors, JSON parse errors),statusCodewill be0. This is handled gracefully but may reduce debugging visibility for non-HTTP errors.web-ui/src/components/checkpoints/CheckpointList.tsx (1)
276-283: Loading state testability improvement.Adding
data-testid="checkpoint-loading"enables E2E tests to wait for loading completion before asserting UI state, directly supporting the PR's goal of fixing test timeouts.tests/e2e/seed-test-data.py (2)
121-182: Project ownership verification addresses authorization issues.This change ensures Project 1 has
user_id=1for the test user, which is critical for the checkpoint API's authorization checks. The upsert pattern (UPDATE then INSERT if no rows affected) is appropriate for handling both fresh installs and re-runs.The verification query at lines 169-181 provides immediate feedback if the update fails, which aids debugging.
1191-1217: Checkpoint verification improves seed data reliability.The post-seeding verification loop confirms that checkpoint records exist and their associated files are present. This catches seeding issues early rather than during test execution.
tests/e2e/test_checkpoint_ui.spec.ts (5)
22-66: API health check provides early failure detection.This pre-test verification ensures the checkpoint API is accessible before running UI tests. If the API is down or misconfigured, tests will fail here with a clear diagnostic rather than timing out in UI tests.
The test correctly uses
loginUser()andgetAuthToken()helpers per coding guidelines.
119-126: Core race condition fix: listener before action.This is the key fix for the test timeouts. By setting up
waitForResponsebefore clicking the checkpoint tab, the listener is guaranteed to capture the API response that fires immediately on click.Previously, the click could trigger the API call and receive a response before any listener was registered, causing indefinite waits.
170-173: Fixed wait avoids flaky timing.The 500ms timeout after API response capture gives the UI time to render. While fixed timeouts can be fragile, this is appropriate here since it's a buffer after confirmed API completion rather than a substitute for proper waits.
The subsequent assertions (lines 181-194) properly wait for actual UI state rather than relying solely on this timeout.
177-191: Improved error state detection prevents silent failures.The loading indicator check and error state detection (lines 186-191) ensure tests fail with actionable messages rather than timing out. This aligns with the coding guideline: "Assert UI elements existence with expect(element).toBeVisible() - fail if missing, never silently pass."
423-436: Consistent race condition fix pattern in new project test.The same fix pattern is correctly applied here: set up the response listener before clicking the tab, then await it after the click. This ensures the empty state test is equally robust against the race condition.
Code Review: PR #260 - fix(e2e): resolve checkpoint UI test timeoutsSummaryThis PR fixes a race condition in checkpoint UI E2E tests by ensuring response listeners are set up before tab clicks trigger API calls. It also improves test data verification, adds error handling, and adds testability features like ✅ Strengths
|
|
|
Review Complete ✓I've posted a comprehensive code review for PR #260: Key Findings✅ Strengths:
📝 Recommendations (non-blocking):
🔒 Security & Performance:
Overall: Approve with minor changes - the race condition fix is solid and follows best practices. Only the null safety check on Review posted at: #260 (comment) |
Changes based on PR #260 review comments: 1. **Blocking fix**: Add null safety for checkpoint.git_commit - Prevents runtime crash if git_commit is null/undefined - Shows 'N/A' as fallback 2. **Accessibility**: Add aria-label to Retry button - Improves screen reader support per CodeRabbit suggestion 3. **Type safety**: Use Playwright's Response type - Replace `Promise<any>` with `Promise<Response>` for better type checking 4. **Maintainability**: Extract timeout constants - Centralized TIMEOUTS object for easier test maintenance - Documents purpose of each timeout value
Review Feedback Addressed ✅I've pushed commit 🔴 Blocking Issue (Fixed)
🟡 Nitpicks (Fixed)
Not Addressed (Intentional)
All tests pass (43 frontend checkpoint tests, 24 backend checkpoint tests). |
Code Review: Additional AssessmentAfter reviewing the files, I can confirm this PR addresses the race condition issue effectively. Most feedback from previous reviews has already been incorporated. ✅ Already Addressed (from previous reviews)
🎯 Current Code Quality AssessmentStrengths:
📝 Minor Suggestions (Non-blocking)1. Error parsing fragility (Informational) 2. Seed data warning threshold (Low priority) 3. Debug console statements (Optional) 🔒 Security & Performance
📋 Overall Verdict✅ Approve - This PR is ready to merge. The race condition fix is solid, error handling improvements add value, and the code follows all project conventions from CLAUDE.md. The changes effectively address the checkpoint UI test timeout issue without introducing new risks. |
|
Review posted: #260 (comment) SummaryThis PR is ready to merge. The race condition fix is sound, and most issues from previous reviews have already been addressed: ✅ Already Fixed:
🎯 Key Strengths:
📝 Minor Non-blocking Suggestions:
No blocking issues found. The changes effectively fix checkpoint UI test timeouts without introducing security or performance risks. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/test_checkpoint_ui.spec.ts (1)
278-307: Tests conditionally skip core assertions based on checkpoint availability.The tests
should show restore confirmation dialog,should display checkpoint diff preview,should display checkpoint metadata, andshould allow deleting checkpointall use runtime count checks to conditionally execute their main assertions:const count = await checkpointItems.count(); if (count > 0) { // assertions for checkpoint feature } else { // verify empty state only }This pattern violates the coding guideline: "Never use test.skip() inside test logic; skip at describe level or use separate test projects for different states." If checkpoints are unavailable (whether by seed failure or runtime issue), these tests pass without validating their stated functionality.
Additionally, the test uses hardcoded
PROJECT_ID = '1'while other E2E test files correctly useTEST_PROJECT_IDS.PLANNINGorTEST_PROJECT_IDS.ACTIVEto guarantee pre-seeded data in the appropriate state.Consider:
- Using
TEST_PROJECT_IDS.ACTIVE(orPLANNING) which has guaranteed checkpoint data seeded- Creating separate test suites for "with checkpoints" vs "empty state" scenarios
- Using describe-level skipping or separate test projects for different states
🧹 Nitpick comments (4)
tests/e2e/test_checkpoint_ui.spec.ts (4)
61-79: Potential double consumption of response body.The response body is read as text on line 62 (
response.text()), thenresponse.json()is called on line 77. In Playwright's API context, calling both on the same response should work sinceAPIResponseallows multiple reads, but this pattern is redundant. Consider parsing JSON once and reusing it.♻️ Suggested simplification
- // Log response body for debugging - const body = await response.text(); - if (status !== 200) { - console.log(`[Health Check] Error response: ${body}`); - } else { - try { - const data = JSON.parse(body); - console.log(`[Health Check] Found ${data.checkpoints?.length || 0} checkpoints`); - } catch { - console.log(`[Health Check] Response: ${body.substring(0, 200)}`); - } - } - - // Verify response - expect(status).toBe(200); - - const data = await response.json(); + // Verify response + expect(status).toBe(200); + + const data = await response.json(); + console.log(`[Health Check] Found ${data.checkpoints?.length || 0} checkpoints`); + expect(data).toHaveProperty('checkpoints'); expect(Array.isArray(data.checkpoints)).toBe(true);
333-333: Inconsistent timeout usage - consider usingTIMEOUTS.UI_RENDER.This hardcoded
1000msdiffers fromTIMEOUTS.UI_RENDER(500ms). The same inconsistency appears on line 454. For maintainability, consider either using the centralized constant or adding a new constant (e.g.,TIMEOUTS.DIFF_RENDER) if a longer delay is intentional.♻️ Suggested change
- await page.waitForTimeout(1000); + await page.waitForTimeout(TIMEOUTS.UI_RENDER);Or add a new constant if longer delay is needed:
const TIMEOUTS = { // ...existing /** Delay for complex UI rendering (diffs, large lists) */ COMPLEX_RENDER: 1000, } as const;
322-330: Consider logging more context when diff API times out.The catch block logs a generic message but doesn't indicate this could be a problem. While the subsequent assertion (line 342) ensures some content appears, silently swallowing the timeout could mask diff API issues. Consider logging at warning level or tracking this for test stability analysis.
365-369: Consider verifying git SHA element presence if it should always exist.The current logic silently passes if no
[data-testid="checkpoint-git-sha"]element exists. If every checkpoint should display git SHA (even as 'N/A'), consider making this a required assertion. If it's truly optional UI, the current approach is acceptable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/e2e/test_checkpoint_ui.spec.tsweb-ui/src/components/checkpoints/CheckpointList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/components/checkpoints/CheckpointList.tsx
🧰 Additional context used
📓 Path-based instructions (2)
tests/**/*.{ts,tsx,test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Never use test.skip() inside test logic; skip at describe level or use separate test projects for different states
Files:
tests/e2e/test_checkpoint_ui.spec.ts
tests/e2e/**/*.{ts,test.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
tests/e2e/**/*.{ts,test.ts}: Use loginUser() helper from tests/e2e/test-utils.ts for authentication in E2E tests
Assert UI elements existence with expect(element).toBeVisible() - fail if missing, never silently pass
Use TEST_PROJECT_IDS.PLANNING for tests requiring pre-seeded planning phase tasks
Use TEST_PROJECT_IDS.ACTIVE for tests requiring pre-seeded active phase with agents
Files:
tests/e2e/test_checkpoint_ui.spec.ts
🧠 Learnings (6)
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.ACTIVE for tests requiring pre-seeded active phase with agents
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use loginUser() helper from tests/e2e/test-utils.ts for authentication in E2E tests
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.PLANNING for tests requiring pre-seeded planning phase tasks
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Assert UI elements existence with expect(element).toBeVisible() - fail if missing, never silently pass
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to tests/**/*.{ts,tsx,test.ts} : Never use test.skip() inside test logic; skip at describe level or use separate test projects for different states
Applied to files:
tests/e2e/test_checkpoint_ui.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Backend Unit Tests
🔇 Additional comments (5)
tests/e2e/test_checkpoint_ui.spec.ts (5)
18-31: LGTM!Well-structured timeout constants with clear JSDoc documentation. Centralizing these values improves maintainability.
134-159: LGTM - Race condition fix correctly implemented.The critical fix is properly implemented: the response listener is registered before clicking the tab, ensuring the API response is captured regardless of how quickly it returns. The logging and error handling in the catch block provide good observability.
180-229: Good addition of loading and error state detection.The explicit handling of loading indicator (lines 196-199) and error state (lines 201-206) with a fail-fast approach improves test reliability. The conditional
count === 0check is reasonable here since the test verifies the listing behavior regardless of data presence.
411-469: LGTM - Good empty state regression test.This test properly validates the fix for the
.sort is not a functionerror by:
- Creating a fresh project without checkpoints
- Capturing console errors during the test
- Asserting no function-related errors occurred
The approach of creating a new project is appropriate here since the test specifically needs an empty checkpoint state.
15-16: ImportTEST_PROJECT_IDSconstant frome2e-configinstead of hardcoding project ID.Follow the established pattern used in all other E2E tests. Import
TEST_PROJECT_IDSfrom./e2e-configand useTEST_PROJECT_IDS.DISCOVERYto match the current default behavior ('1') while maintaining consistency across the test suite.Current code
const FRONTEND_URL = process.env.FRONTEND_URL || 'http://localhost:3001'; const PROJECT_ID = process.env.E2E_TEST_PROJECT_ID || '1';Should be:
import { FRONTEND_URL, BACKEND_URL, TEST_PROJECT_IDS } from './e2e-config'; const PROJECT_ID = TEST_PROJECT_IDS.DISCOVERY;The checkpoint UI tests verify generic checkpoint functionality that works across all project phases. Using the DISCOVERY constant (which defaults to '1') preserves existing behavior while aligning with test data conventions used throughout the E2E suite.
⛔ Skipped due to learnings
Learnt from: CR Repo: frankbria/codeframe PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T23:33:31.895Z Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.ACTIVE for tests requiring pre-seeded active phase with agentsLearnt from: CR Repo: frankbria/codeframe PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T23:33:31.895Z Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use TEST_PROJECT_IDS.PLANNING for tests requiring pre-seeded planning phase tasksLearnt from: CR Repo: frankbria/codeframe PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T23:33:31.895Z Learning: Applies to tests/e2e/**/*.{ts,test.ts} : Use loginUser() helper from tests/e2e/test-utils.ts for authentication in E2E testsLearnt from: CR Repo: frankbria/codeframe PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T23:33:31.895Z Learning: Applies to tests/**/*.{ts,tsx,test.ts} : Never use test.skip() inside test logic; skip at describe level or use separate test projects for different statesLearnt from: CR Repo: frankbria/codeframe PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-11T23:33:31.895Z Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
1 participant
Summary
Root Cause
The tests were waiting for
/checkpointsAPI response AFTER clicking the checkpoint tab, but the API call fires immediately on tab click, causing a race condition where the response was already received before the listener was set up.Changes
Test plan
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.