fix(api): align task approval request body with backend contract #240
Conversation
The frontend was sending { task_ids: [...] } but the backend expects
{ approved: bool, excluded_task_ids: [...] }, causing HTTP 422
validation errors.
Changes:
- Update approveTaskBreakdown API function to accept both selectedTaskIds
and allTaskIds parameters to compute excluded IDs
- Convert from frontend selection model to backend exclusion model
- Extract numeric IDs from string task IDs (e.g., 'task-4' -> 4)
- Update TaskReview component to pass all task IDs for exclusion calculation
- Add comprehensive unit tests for API contract validation
- Add E2E test suite for task approval API contract verification
The fix bridges the semantic gap between:
- Frontend: tracks which tasks ARE selected (positive selection)
- Backend: expects which tasks to EXCLUDE (negative selection)
WalkthroughFrontend approval flow now sends both selected and all task IDs; the client/API compute Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TaskReview
participant API as projectsApi.approveTaskBreakdown
participant Backend as /api/projects/{id}/tasks/approve
User->>UI: Click "Approve" with selectedTaskIds
UI->>UI: collect/compute allTaskIds
UI->>API: approveTaskBreakdown(projectId, selectedTaskIds, allTaskIds)
API->>API: compute excluded_task_ids = allTaskIds - selectedTaskIds (numeric extraction)
API->>Backend: POST { approved: true, excluded_task_ids }
Backend-->>API: 200/422 + payload
API-->>UI: return response or throw validation error
UI-->>User: show success or validation errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 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 |
Code Review - PR #240Excellent bug fix and comprehensive testing! This PR properly addresses the API contract mismatch between frontend and backend. Here's my detailed review: ✅ Strengths
🔍 Code Quality Observations1. Potential Bug: ID Extraction Logic (Minor)// Current implementation (api.ts:122-126)
const numericPart = id.replace(/\D/g, '');
return parseInt(numericPart, 10);Issue: This regex removes ALL non-digit characters, which could cause issues with hyphenated IDs:
Suggestion: Use a more specific pattern: const match = id.match(/-(\d+)$/) || id.match(/^(\d+)$/);
if (\!match) return NaN;
return parseInt(match[1], 10);This ensures only IDs like 2. E2E Test Coverage Gaps (Acknowledged Issue)The E2E tests have some limitations (related to Issue #172): // test_task_approval.spec.ts:96-97
if (await taskReview.isVisible().catch(() => false) &&
await approveButton.isVisible().catch(() => false)) {Observations:
Note: This is consistent with existing E2E patterns and documented in Issue #172. Not a blocker for this PR. 3. Type Safety Enhancement (Nice-to-Have)// TaskReview.tsx:210
const allTaskIds = getAllTaskIds(issues);The const allTaskIds = getAllTaskIds(issues);
if (allTaskIds.length === 0) {
console.warn('No tasks available for approval');
return;
}This prevents calling the API with empty arrays when the UI is in an unexpected state. 🛡️ Security Review✅ No security concerns identified:
🎯 Performance Considerations✅ Efficient implementation:
📝 Test Coverage AssessmentUnit Tests: ✅ Excellent
E2E Tests: ✅ Comprehensive
The negative test (test_task_approval.spec.ts:336-358) that verifies 422 for incorrect format is particularly valuable for regression prevention. 🎬 RecommendationsPriority 1 (Before Merge):
Priority 2 (Future Enhancement):
✨ SummaryThis is a high-quality fix with excellent test coverage. The implementation correctly bridges the semantic gap between frontend and backend models. The only suggested change is strengthening the ID extraction regex to be more defensive against malformed task IDs. Verdict: ✅ Approve with minor suggestion (ID extraction pattern) Great work on the thorough testing and clear documentation! 🚀 |
Updated Review - PR #240Good progress! I see the second commit addressing browser-specific navigation errors. ✅ Second Commit Review:
|
![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 (3)
tests/e2e/test_task_approval.spec.ts (3)
63-140: Comprehensive contract validation that addresses the original bug.The test correctly validates all aspects of the API contract:
- Required
approvedboolean field- Required
excluded_task_idsarray with numeric IDs- Absence of the buggy
task_idsfield- Graceful handling when project isn't in planning phase
The defensive programming with visibility checks, error handling, and conditional skipping is excellent.
♻️ Optional: Extract navigation helper to reduce duplication
The navigation pattern (goto project → wait → click tasks tab) is repeated across tests. Consider extracting it:
async function navigateToTasksTab(page: Page, projectId: string) { await page.goto(`${FRONTEND_URL}/projects/${projectId}`); await page.waitForLoadState('networkidle'); const tasksTab = page.locator('[data-testid="tasks-tab"]'); await tasksTab.waitFor({ state: 'visible', timeout: 10000 }).catch(() => {}); if (await tasksTab.isVisible()) { await tasksTab.click(); return true; } return false; }This would improve maintainability and make the test intent clearer.
287-291: Consider adding error monitoring for consistency.The first test suite includes
setupErrorMonitoringandcheckTestErrors, but the direct API test suite omits them. While direct API tests may have fewer UI errors, adding consistent error monitoring would help catch unexpected issues.♻️ Suggested addition for consistency
test.describe('Task Approval API Contract - Direct API Tests', () => { test.beforeEach(async ({ context, page }) => { + const errorMonitor = setupErrorMonitoring(page); + (page as ExtendedPage).__errorMonitor = errorMonitor; + await context.clearCookies(); await loginUser(page); }); + + test.afterEach(async ({ page }) => { + checkTestErrors(page, 'Direct API test', [ + 'net::ERR_ABORTED', + 'Failed to fetch RSC payload', + 'NS_BINDING_ABORTED', + 'Load request cancelled' + ]); + });
298-333: Well-designed contract validation test.The test correctly validates that the request format is accepted (non-422 status) regardless of business logic outcomes. The pragmatic approach of accepting multiple status codes (200/400/403/404) focuses on contract compliance rather than operation success.
♻️ Optional: Consider using Playwright's storageState for auth
The current approach retrieves the auth token from localStorage, which works but couples the test to the storage implementation. Consider using Playwright's
storageStatefeature or a helper that abstracts auth token retrieval:// In test-utils.ts export async function getAuthToken(page: Page): Promise<string | null> { return await page.evaluate(() => localStorage.getItem('auth_token')); } // In test const authToken = await getAuthToken(page);This provides better encapsulation and makes future auth mechanism changes easier.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/test_task_approval.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
tests/e2e/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Files:
tests/e2e/test_task_approval.spec.ts
🧠 Learnings (3)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/test_task_approval.spec.ts
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/test_task_approval.spec.ts
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
tests/e2e/test_task_approval.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). (4)
- GitHub Check: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (5)
tests/e2e/test_task_approval.spec.ts (5)
1-23: Excellent test documentation and proper test utilities usage.The header clearly explains the bug this test prevents, and the imports correctly use
loginUser()from test-utils as required by the coding guidelines for E2E tests.
31-47: Well-structured test lifecycle hooks with appropriate error filtering.The error monitoring setup and filtered error list (navigation aborts, RSC fetch failures) prevent false positives from transient UI state changes, which aligns with the commit message about filtering navigation errors.
147-212: Effective validation error handling test.The mocked 422 response uses the correct Pydantic validation error format, and the UI error detection logic checks multiple patterns (CSS classes, ARIA roles, error text) to ensure graceful degradation.
219-279: Excellent integration test with clear conditional execution.The conditional test name clearly communicates the API key requirement, and the fail-fast approach on 422 with a detailed error message makes debugging straightforward. The comprehensive response validation ensures the success path returns expected fields.
340-369: Excellent negative test using the actual buggy format.This test documents the expected rejection behavior by using the exact incorrect format that caused the original bug (
task_idsfield). It serves as effective regression prevention and validates that the backend properly rejects malformed requests.
Replace greedy regex that strips all non-digits with a defensive pattern that only accepts valid ID formats: - 'prefix-digits' (e.g., 'task-4', 'issue-25') - Pure digits (e.g., '42') This prevents silently mangling malformed IDs: - 'task-1-2-3' now extracts '3' (last number after hyphen) - 'abc123def' now returns NaN and is filtered out - 'invalid' now returns NaN and is filtered out Added tests to verify defensive behavior.
Final Review - PR #240 ✅Great work addressing the previous feedback! This PR is now ready to merge. 🎯 Changes Since Last ReviewThe developer has implemented my previous recommendation to improve the ID extraction regex: Before: const numericPart = id.replace(/\D/g, ''); // Strips ALL non-digitsAfter (Current): const match = id.match(/-(\d+)$/) || id.match(/^(\d+)$/);
if (\!match) return NaN;
return parseInt(match[1], 10);This change is exactly what I suggested and properly prevents malformed IDs like ✅ Code Quality Assessment1. ID Extraction Logic ✅ FIXEDThe regex now only accepts:
The test in 2. API Contract Implementation ✅ CORRECTThe fix properly implements the backend contract:
3. Test Coverage ✅ COMPREHENSIVE
4. Security ✅ NO CONCERNS
5. Performance ✅ EFFICIENT
📋 Final Checks
🎉 SummaryThis PR demonstrates excellent software engineering:
Recommendation: APPROVE AND MERGE 🚀 No further changes required. The code is production-ready. 📝 Minor Observation (Non-Blocking)The test comment at
This is actually correct behavior for the suffix pattern - it extracts the trailing number after the last hyphen. If the project uses IDs like If multi-hyphen IDs with concatenated numbers are needed in the future, consider this pattern: // For 'task-1-2-3' → 123 (concatenate all digits)
const numericPart = id.replace(/\D/g, '');
return numericPart ? parseInt(numericPart, 10) : NaN;But for now, the current implementation aligns with the project's ID format conventions. ✅ |
Final Review - PR #240 ✅Status: APPROVED 🎉 Excellent work addressing all feedback! The PR now implements a complete and robust fix for the task approval API contract mismatch. 📝 Commit SummaryCommit 1 (7b72f2d): Core fix aligning frontend/backend contract ✅ All Previous Recommendations Addressed1. ID Extraction Regex - IMPLEMENTED ✅The defensive pattern from my previous review has been correctly implemented in web-ui/src/lib/api.ts:125-126:
2. Comprehensive Test Coverage - EXCELLENT ✅New test in api.test.ts:281-306 validates defensive behavior with perfect edge case coverage for malformed input handling. 3. Cross-Browser Compatibility - RESOLVED ✅Commit 2 added proper error filtering for all major browsers (Chromium, Firefox, WebKit/Safari). 🎯 Final Code Quality Assessment
🚀 Key Improvements Over Original Code
📊 Test Results
💡 Why This PR is Production-ReadyThis fix demonstrates excellent software engineering practices:
✨ Final VerdictAPPROVED - Ready to Merge ✅ This PR successfully:
No further changes required. Excellent work! 🚀 Reviewer: Claude Code |
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)
web-ui/src/lib/api.ts (1)
5-5: Upgrade axios to 1.13.2 to address critical security vulnerabilities.Axios 1.6.5 is affected by multiple known CVEs:
- CVE-2025-27152 (SSRF vulnerability in versions ≤1.7.9, fixed in 1.8.2)
- CVE-2023-45857 (XSRF cookie leakage)
- DoS via data: URLs (affects versions <1.12.0, fixed in 1.12.0+)
Upgrade to the latest stable version 1.13.2 (November 2025) to resolve all vulnerabilities.
🧹 Nitpick comments (1)
web-ui/src/lib/api.ts (1)
118-129: Exclusion logic is correct; minor comment precision note.The Set-based exclusion algorithm is efficient and correct. The defensive regex patterns with NaN filtering safely handle edge cases.
The comment on line 124 states "only accept 'prefix-digits' or pure digits", but the pattern
/-(\d+)$/would technically also match strings like "-4" (hyphen-digits without a prefix). In practice, this shouldn't be an issue since task IDs are controlled by the backend, and the NaN filtering provides a safety net.📝 Optional: Make regex comment more precise
If you want the comment to exactly match the regex behavior, consider:
- // Extract numeric ID from strings like 'task-4' or just '4' - // Defensive: only accept 'prefix-digits' or pure digits, reject malformed IDs + // Extract numeric ID from strings like 'task-4' or just '4' + // Defensive: accept strings ending with hyphen-digits or pure digits, reject malformed IDsAlternatively, to truly enforce "prefix-digits", require at least one character before the hyphen:
- const match = id.match(/-(\d+)$/) || id.match(/^(\d+)$/); + const match = id.match(/^.+-(\d+)$/) || id.match(/^(\d+)$/);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
web-ui/__tests__/lib/api.test.tsweb-ui/src/lib/api.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/tests/lib/api.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/lib/api.ts
web-ui/src/lib/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Frontend API files must use const API_BASE_URL = process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8080' pattern without hardcoded production URLs or different fallback ports
Files:
web-ui/src/lib/api.ts
🧠 Learnings (1)
📚 Learning: 2025-11-25T19:08:54.154Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T19:08:54.154Z
Learning: Applies to specs/*/tasks.md : Feature task files (tasks.md) must include phase-by-phase task breakdown with unique task identifiers (T001, T002, etc.), acceptance criteria per task, beads issue references, and estimated effort
Applied to files:
web-ui/src/lib/api.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 (3)
web-ui/src/lib/api.ts (3)
105-116: LGTM! Clear documentation of the exclusion model conversion.The JSDoc accurately describes the frontend-to-backend contract transformation and provides helpful context for future maintainers.
117-117: LGTM! Type-safe function signature.The explicit typing of all parameters follows TypeScript strict mode requirements and clearly communicates the function's contract.
131-140: LGTM! API call correctly implements the backend contract.The payload structure with
approved: trueandexcluded_task_idsmatches the backend expectations described in the PR objectives, fixing the HTTP 422 error. The return type is properly typed for type safety.
Summary
{ task_ids: [...] }but backend expected{ approved: bool, excluded_task_ids: [...] }Changes
web-ui/src/lib/api.ts): UpdatedapproveTaskBreakdownto compute excluded task IDs from selected/all task arraysweb-ui/src/components/TaskReview.tsx): Pass all task IDs for exclusion calculationtests/e2e/test_task_approval.spec.ts): New test suite for API contract verificationRoot Cause
The frontend used a selection model (track selected tasks) while the backend uses an exclusion model (specify excluded tasks). The fix bridges this semantic gap by computing
excluded_task_ids = allTaskIds - selectedTaskIds.Test plan
npx playwright test test_task_approval.spec.tsSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.