fix(state): load tasks from API for returning users (#231) - #243
Conversation
PROBLEM: Users who navigate to a project AFTER missing WebSocket events didn't see tasks in the UI. The AgentStateProvider intentionally skipped loading tasks from API, expecting them only via WebSocket. ROOT CAUSE: - Tasks were only populated via WebSocket events (TASK_ASSIGNED, etc.) - AgentStateProvider had an empty useEffect for tasksData - Returning users (page refresh, login later) never received WebSocket history SOLUTION: - Added TASKS_LOADED action to agentReducer - AgentStateProvider now dispatches TASKS_LOADED when tasksData arrives from SWR - Tasks are loaded from API on mount, ensuring state reconciliation CHANGES: - web-ui/src/types/agentState.ts: Add TasksLoadedAction type - web-ui/src/reducers/agentReducer.ts: Add TASKS_LOADED case - web-ui/src/components/AgentStateProvider.tsx: Dispatch TASKS_LOADED on mount - web-ui/src/components/TaskList.tsx: Add data-status attribute for testing - web-ui/src/components/Dashboard.tsx: Add data-testid and data-phase to status badge - tests/e2e/test_returning_user.spec.ts: New E2E tests for returning user scenarios - tests/e2e/test-utils.ts: Add state verification helpers - tests/e2e/README.md: Document returning user vs late-joining user patterns TESTING: - 6 new E2E tests covering returning user scenarios - All tests pass on Chromium - Existing unit tests (58) still pass
WalkthroughAdds returning-user support: API-first task loading and validation for users who missed WebSocket events. Introduces E2E docs, new Playwright returning-user tests and helpers (including WebSocket-blocking), app-side task parsing/transform, TASKS_LOADED action/reducer, and DOM test attributes to enable state-reconciliation verification. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant WebSocket
participant API
participant Redux
User->>Browser: Open app (returning user)
Browser->>WebSocket: Attempt WebSocket upgrade
WebSocket--x Browser: Upgrade blocked/unavailable
Browser->>API: GET /api/projects/{projectId}/tasks (with auth)
API-->>Browser: Return task list and project state
Browser->>Browser: Validate & transform tasks (isValidTaskResponse, transformAPITask)
Browser->>Redux: Dispatch TASKS_LOADED with tasks
Redux-->>Browser: State updated
Browser->>Browser: Render UI (data-phase / data-status attributes)
Browser-->>User: Display reconciled project/task state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
remote: Duplicate header: "Authorization" |
Load tasks from API on mount for returning users and dispatch
|
|
PR Review: Fix Returning User Task State Loading Summary: Excellent fix for a critical UX issue! The PR correctly identifies and solves the problem where returning users do not see tasks because AgentStateProvider only loaded them via WebSocket events. Strengths:
Observations & Minor Suggestions:
Security & Performance: No concerns identified. No SQL injection or XSS risks. Auth properly handled. Performance impact minimal. Code Quality: Follows repository patterns. TypeScript types added. Immutable state updates. No backwards compatibility breaks. Proper cleanup in tests. Verdict: LGTM with minor suggestions! This is a well-crafted fix with excellent testing. Recommendations: Consider addressing the duplicate blockWebSocket functions. Add unit test for TASKS_LOADED reducer case. Otherwise, ready to merge! Great work on the clear documentation and comprehensive E2E coverage! |
![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
🤖 Fix all issues with AI agents
In @tests/e2e/test_returning_user.spec.ts:
- Around line 387-423: The test 'should show quality gates as passed for
completed project' calls getAuthenticatedRequest() but never uses the returned
request and token variables; either remove the unused destructuring (delete
request and token from the const assignment) or use them to perform the same API
verification pattern as other tests (use the returned request to call the
backend for project or tasks status and assert the API response matches the UI).
Update the const assignment that calls getAuthenticatedRequest, or add an API
call using request (and include token where required) to validate quality gate
data before asserting failedCount.
- Around line 516-559: The test extracts unused auth variables from
getAuthenticatedRequest(page) causing lint warnings; remove the unused
destructuring by calling the helper without assigning (e.g., await
getAuthenticatedRequest(page);) or explicitly ignore them (e.g., const {
request: _request, token: _token } = await getAuthenticatedRequest(page);),
updating the test named "should load complete state from API endpoints without
WebSocket @returning-user" to reference only needed values and avoid unused
symbols.
🧹 Nitpick comments (3)
tests/e2e/test_returning_user.spec.ts (3)
21-28: Remove unused importwaitForAPIResponse.The
waitForAPIResponsefunction is imported but never used in this file.🧹 Suggested fix
import { loginUser, setupErrorMonitoring, checkTestErrors, ExtendedPage, - waitForAPIResponse, } from './test-utils';
62-157: Consider using shared helpers fromtest-utils.tsto reduce duplication.The local helper functions
blockWebSocket,verifyTaskState, andverifyProjectPhaseare nearly identical to the newly added helpers intest-utils.ts:
blockWebSocket→blockWebSocketConnectionsverifyTaskState→verifyTaskStateFromAPIverifyProjectPhase→verifyProjectPhaseFromAPIUsing the shared helpers would reduce code duplication and ensure consistent behavior across tests.
♻️ Suggested refactor
import { loginUser, setupErrorMonitoring, checkTestErrors, ExtendedPage, - waitForAPIResponse, + blockWebSocketConnections, + verifyTaskStateFromAPI, + verifyProjectPhaseFromAPI, + getAuthToken, } from './test-utils'; -/** - * Helper to get an authenticated API request context - */ -async function getAuthenticatedRequest(page: Page): Promise<{ request: APIRequestContext; token: string }> { - // ... entire function -} - -/** - * Block WebSocket connections to simulate returning user scenario - */ -async function blockWebSocket(page: Page): Promise<() => Promise<void>> { - // ... entire function -} - -/** - * Verify task counts from API match expected state - */ -async function verifyTaskState( - // ... entire function -} - -/** - * Verify project phase from API - */ -async function verifyProjectPhase( - // ... entire function -}Then update usages throughout the tests to use the imported helpers.
222-226: Consider replacing fixed timeouts with explicit wait conditions.Multiple
waitForTimeout(500)calls are used after tab clicks. While sometimes necessary for UI transitions, explicit waits are more reliable. Consider waiting for specific state changes instead.💡 Example approach
// Click on Tasks tab to see task list const tasksTab = page.locator('[data-testid="tasks-tab"]'); await tasksTab.click(); - await page.waitForTimeout(500); + // Wait for tab panel content to be ready + await page.locator('[data-testid="tasks-panel"]').waitFor({ state: 'visible' });This approach is more deterministic and can reduce test flakiness in CI environments.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/e2e/README.mdtests/e2e/test-utils.tstests/e2e/test_returning_user.spec.tsweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/components/Dashboard.tsxweb-ui/src/components/TaskList.tsxweb-ui/src/reducers/agentReducer.tsweb-ui/src/types/agentState.ts
🧰 Additional context used
📓 Path-based instructions (8)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/Dashboard.tsxweb-ui/src/reducers/agentReducer.tsweb-ui/src/components/TaskList.tsxweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/Dashboard.tsxweb-ui/src/components/TaskList.tsxweb-ui/src/components/AgentStateProvider.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/Dashboard.tsxweb-ui/src/components/TaskList.tsxweb-ui/src/components/AgentStateProvider.tsx
web-ui/src/components/Dashboard.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Files:
web-ui/src/components/Dashboard.tsx
web-ui/src/reducers/agentReducer.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
Files:
web-ui/src/reducers/agentReducer.ts
**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Documentation files must be sized to fit in a single agent context window (spec.md ~200-400 lines, plan.md ~300-600 lines, tasks.md ~400-800 lines)
Files:
tests/e2e/README.md
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_returning_user.spec.tstests/e2e/test-utils.ts
web-ui/src/components/AgentStateProvider.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Files:
web-ui/src/components/AgentStateProvider.tsx
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/Dashboard.tsxweb-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/Dashboard.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.tsx : Replace all icon usage with Hugeicons (hugeicons/react) and do not mix with lucide-react
Applied to files:
web-ui/src/components/Dashboard.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Applied to files:
web-ui/src/components/Dashboard.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
Applied to files:
web-ui/src/reducers/agentReducer.tsweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/contexts/AgentStateContext.ts : Use context-based state management with React Context + useReducer pattern for Dashboard with AgentStateContext, agentReducer, and useAgentState hook
Applied to files:
web-ui/src/reducers/agentReducer.tsweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/reducers/agentReducer.tstests/e2e/README.mdweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Applied to files:
tests/e2e/README.mdtests/e2e/test_returning_user.spec.tstests/e2e/test-utils.ts
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
tests/e2e/README.mdtests/e2e/test_returning_user.spec.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
🧬 Code graph analysis (2)
tests/e2e/test_returning_user.spec.ts (4)
tests/e2e/e2e-config.ts (2)
FRONTEND_URL(14-14)BACKEND_URL(11-11)codeframe/cli/project_commands.py (1)
tasks(255-317)tests/e2e/test-utils.ts (3)
setupErrorMonitoring(64-104)ExtendedPage(25-27)checkTestErrors(192-216)codeframe/cli.py (1)
agents(164-169)
web-ui/src/types/agentState.ts (2)
web-ui/src/types/api.ts (1)
Task(30-72)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (1)
Task(97-105)
⏰ 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 (13)
tests/e2e/README.md (1)
723-806: Excellent documentation of the returning user pattern.This section clearly distinguishes between late-joining users (who may catch some WebSocket events) and returning users (who receive no events). The before/after code examples effectively illustrate the fix, and the helper function documentation will be valuable for writing future tests.
web-ui/src/components/TaskList.tsx (1)
84-84: LGTM! DOM attribute for E2E testing.The
data-statusattribute exposes task status in the DOM, enabling the new returning-user E2E tests to verify task state without relying on WebSocket events.web-ui/src/components/Dashboard.tsx (1)
359-365: LGTM! DOM attributes for E2E testing.The
data-testidanddata-phaseattributes enable E2E tests to verify project state, consistent with the pattern used in TaskList.web-ui/src/components/AgentStateProvider.tsx (1)
119-144: Fix addresses the core issue, but verify SWR revalidation behavior.The TASKS_LOADED dispatch correctly initializes tasks from the API, solving the returning-user problem described in #231. However, since the effect depends on
tasksData, it will re-run if SWR revalidates the data (even thoughrevalidateOnFocusandrevalidateOnReconnectare false). This could overwrite tasks that have been updated via WebSocket.Consider adding a guard to only dispatch TASKS_LOADED once:
Details
SWR revalidateOnFocus false still revalidates The search results confirm that specifying an empty array ensures the effect runs only once for useEffect. However, the current implementation has `[tasksData]` as the dependency, so it will re-run if tasksData changes. Given that the SWR config has `refreshInterval: 0`, `revalidateOnFocus: false`, and `revalidateOnReconnect: false`, tasksData should only change once on initial load in normal circumstances.However, there's a design consideration: should TASKS_LOADED only run on first mount or every time tasksData updates? Let me provide a balanced comment.
Based on SWR configuration (
refreshInterval: 0,revalidateOnFocus: false,revalidateOnReconnect: false), tasksData should typically only update once. However, if the cache is manually invalidated or mutated, this effect will re-dispatch TASKS_LOADED and potentially overwrite WebSocket-updated tasks.web-ui/src/reducers/agentReducer.ts (1)
102-112: Consider using last-write-wins merge or state guard for TASKS_LOADED.The TASKS_LOADED action replaces all tasks without merging. If WebSocket task events (TASK_ASSIGNED, TASK_STATUS_CHANGED) arrive before the API response completes, they will be lost when TASKS_LOADED overwrites the entire task list. While SWR typically completes faster than WebSocket connection, this timing is not guaranteed.
Either merge incoming tasks with existing state:
case 'TASKS_LOADED': { const existingIds = new Set(state.tasks.map(t => t.id)); const newTasks = action.payload.filter(t => !existingIds.has(t.id)); newState = { ...state, tasks: [...state.tasks, ...newTasks], }; break; }Or dispatch only when state is empty to avoid overwriting WebSocket-populated tasks.
web-ui/src/types/agentState.ts (1)
168-176: LGTM! Well-structured action type following established patterns.The
TasksLoadedActioninterface correctly mirrors the existingAgentsLoadedActionpattern and integrates cleanly into the discriminated union. The JSDoc clearly explains the use case for returning users.tests/e2e/test_returning_user.spec.ts (1)
159-182: Well-structured test setup with comprehensive error monitoring.The test setup properly handles authentication via
loginUser, establishes error monitoring, and theafterEachhook appropriately filters expected errors (WebSocket-related) while still catching unexpected failures. Good use of test tags (@smoke,@returning-user) for selective test execution. Based on learnings, this follows the recommended pattern of usingloginUser()helper from test-utils.ts.tests/e2e/test-utils.ts (6)
720-734: LGTM! Clear interface definition for expected task state.The
ExpectedTaskStateinterface provides a well-typed structure for specifying expected task counts by status. All fields are optional, allowing flexible partial assertions.
754-804: Well-implemented API verification helper with clear error reporting.The function properly:
- Retrieves auth token from page context
- Makes authenticated API request
- Counts tasks by status
- Provides helpful error messages on assertion failures
- Returns both raw data and counts for flexible usage
814-839: LGTM! Consistent API verification pattern.Follows the same pattern as
verifyTaskStateFromAPIwith clear error handling and returns the project object for additional assertions if needed.
851-891: Good design: non-throwing DOM verification with detailed results.The function returns an object with
{ actualCounts, passed, errors }rather than throwing, which gives callers flexibility in how to handle mismatches. The locators support multiple data attribute patterns for compatibility with different UI implementations.
901-923: LGTM! Useful helper for completion state verification.Returns a comprehensive result including
isComplete,hasActiveWork, anddetailsfor debugging, making it easy to verify project completion state from the UI.
925-954: Well-documented WebSocket blocking utility.The function clearly documents the critical requirement to call it before navigation. The cleanup function pattern ensures tests can restore WebSocket connectivity, preventing test isolation issues.
|
remote: Duplicate header: "Authorization" |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| case 'TASKS_LOADED': { | ||
| newState = { | ||
| ...state, | ||
| tasks: action.payload, | ||
| }; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Suggestion: TASKS_LOADED shouldn’t overwrite state.tasks; it can race with WebSocket updates and reintroduce stale data. Merge by id, prefer newer timestamps, and add tasks not present.
| case 'TASKS_LOADED': { | |
| newState = { | |
| ...state, | |
| tasks: action.payload, | |
| }; | |
| break; | |
| } | |
| case 'TASKS_LOADED': { | |
| const incomingById = new Map(action.payload.map((t) => [t.id, t])); | |
| const mergedTasks = state.tasks.map((t) => { | |
| const incoming = incomingById.get(t.id); | |
| return incoming && incoming.timestamp >= t.timestamp ? incoming : t; | |
| }); | |
| const newTasks = action.payload.filter((t) => !state.tasks.some((st) => st.id === t.id)); | |
| newState = { | |
| ...state, | |
| tasks: [...mergedTasks, ...newTasks], | |
| }; | |
| break; | |
| } |
🚀 Want me to fix this? Reply ex: "fix it for me".
Follow-up Review: CI Fix and Outstanding RecommendationsNew Changes Since Last ReviewCI Workflow Fix (f30be68): ✅ CI Fix Approved - The workflow changes are appropriate and solve the authentication conflict issue. Outstanding Recommendations from Previous ReviewMy previous review identified several minor improvements. Here's the current status: 1. Duplicate Code - blockWebSocket() Function
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/opencode-review.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (2)
.github/workflows/opencode-review.yml (2)
41-41: LGTM: Clearer step name.The updated name accurately reflects the expanded credential-clearing operations.
46-60: Line 60 is intentional, not redundant; the credential clearing strategy is sound.The difference between unsetting and setting to empty string is deliberate:
git config --unset-all credential.helper(lines 49–50) removes the config entry, whilegit config credential.helper ""(line 60) explicitly disables credential helpers to prevent system-wide defaults from overriding. This hardening step prevents conflicts with GitHub Actions' implicit GITHUB_TOKEN.The aggressive clearing is justified because the subsequent
anomalyco/opencode/github@latestaction doesn't require git credentials—it uses environment variables and the implicit GITHUB_TOKEN for GitHub API access. No interference with later steps.
…tion - Add TypeScript interfaces for API task responses (APITaskResponse) - Add isValidTaskResponse() and transformAPITask() validation functions - Update AgentStateProvider to validate tasks before dispatching TASKS_LOADED - Remove duplicate blockWebSocket function, use shared blockWebSocketConnections - Fix test assertion to verify exact task count (5) matches API - Add unit tests for TASKS_LOADED reducer action (4 test cases) - Fix unused variable warnings in test functions Resolves code review feedback on PR #243
|
remote: Duplicate header: "Authorization" |
✅ Follow-up Review: All Recommendations Addressed!SummaryExcellent work! All issues from my previous review have been successfully addressed in the latest commits. The PR is now in great shape and ready to merge. Changes Verified1. ✅ Duplicate Code Removed - FIXED
2. ✅ Unit Tests Added - FIXED
3. ✅ Test Assertion Strengthened - FIXED
4. ✅ Validation Added - FIXED
Code Quality Assessment✅ All Checks Passing:
Final Verdict🎉 APPROVED - Ready to Merge! This PR successfully fixes issue #231 with:
The code quality is excellent and all previous concerns have been resolved. Great work on iterating quickly on the feedback! 🚀 Recommendation: Merge when ready - no outstanding issues. |
Updated Review: Returning User Task State Loading FixChanges Since Last Review ✅Great progress! The following recommendations from my previous review have been addressed:
Current PR StatusCode Quality: Excellent ⭐ The PR successfully solves the returning user state reconciliation issue (Issue #231) with:
Detailed Review✅ Core Implementation (agentReducer.ts, AgentStateProvider.tsx)Strengths:
Observations:
✅ E2E Testing (test_returning_user.spec.ts)Strengths:
Minor observations:
✅ Test Utilities (test-utils.ts)Strengths:
Type Safety Note: ✅ Documentation (README.md)Strengths:
Security & Performance
Test Coverage
RecommendationLGTM - Ready to Merge! 🚀 All previous recommendations have been addressed. The PR:
This is a well-crafted fix with excellent testing discipline. The distinction between returning user and late-joining user scenarios is clearly documented and properly tested. Great work on addressing all feedback! 🎉 |
Updated Review - All Recommendations Addressed!Great work addressing the feedback from my previous reviews! Here's the status update: Previously Recommended Items - ALL RESOLVED
Code Quality AssessmentStrengths:
Test Coverage:
Architecture:
Security and Performance
Final Verdict: APPROVED - Ready to Merge!This PR successfully fixes a critical UX issue where returning users could not see task state because AgentStateProvider only loaded tasks via WebSocket events. The fix is well-implemented, thoroughly tested, and all previous recommendations have been addressed. Key Achievements:
The distinction between returning user (no WebSocket events) vs late-joining user (partial events) is now well-documented and properly tested. Great work! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @web-ui/src/components/AgentStateProvider.tsx:
- Around line 120-149: The effect that loads tasks only dispatches TASKS_LOADED
when validTasks.length > 0, which leaves stale tasks when the API returns an
empty list; change the useEffect (the block using tasksData,
isValidTaskResponse, transformAPITask, validTasks and dispatch) to always
dispatch type 'TASKS_LOADED' with the validated/transformed validTasks array
(including an empty array) whenever tasksData.data.tasks is present and an
array, removing the conditional `if (validTasks.length > 0)` so the store is
cleared/reset on an empty API response.
🧹 Nitpick comments (2)
web-ui/src/components/AgentStateProvider.tsx (1)
22-24: Imports are fine; please keepTaskparsing fully type-narrowed (avoid casts).tests/e2e/test_returning_user.spec.ts (1)
57-131: Prefer shared API verification helpers from./test-utilsto reduce duplication/drift.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/opencode-review.ymltests/e2e/test_returning_user.spec.tsweb-ui/__tests__/reducers/agentReducer.test.tsweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/src/types/agentState.ts
🧰 Additional context used
📓 Path-based instructions (5)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/AgentStateProvider.tsx
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/AgentStateProvider.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/AgentStateProvider.tsx
web-ui/src/components/AgentStateProvider.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Files:
web-ui/src/components/AgentStateProvider.tsx
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_returning_user.spec.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
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
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/contexts/AgentStateContext.ts : Use context-based state management with React Context + useReducer pattern for Dashboard with AgentStateContext, agentReducer, and useAgentState hook
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/__tests__/reducers/agentReducer.test.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/__tests__/reducers/agentReducer.test.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to 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_returning_user.spec.tsweb-ui/__tests__/reducers/agentReducer.test.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_returning_user.spec.tsweb-ui/__tests__/reducers/agentReducer.test.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocket.ts : Implement WebSocket connections with authentication token passed as query parameter (?token=TOKEN)
Applied to files:
tests/e2e/test_returning_user.spec.ts
🧬 Code graph analysis (1)
web-ui/__tests__/reducers/agentReducer.test.ts (3)
web-ui/src/reducers/agentReducer.ts (2)
getInitialState(22-31)agentReducer(76-396)web-ui/test-utils/agentState.fixture.ts (2)
createMockTask(41-50)createInitialAgentState(80-90)web-ui/src/types/agentState.ts (1)
TasksLoadedAction(225-228)
⏰ 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 (4)
.github/workflows/opencode-review.yml (2)
40-40: Good security practice!Adding
persist-credentials: falseprevents the GitHub token from being persisted in.git/config, which helps avoid auth conflicts in subsequent steps and follows security best practices.
42-61: Thorough credential cleanup addresses auth conflicts.The expanded cleanup commands comprehensively remove all GitHub-related git credentials at both global and local scopes. While some commands might be more extensive than strictly necessary, this defensive approach safely prevents auth conflicts during CI runs without introducing any issues.
web-ui/__tests__/reducers/agentReducer.test.ts (2)
24-39: Good additive coverage for the newTasksLoadedActionwiring.
104-182:TASKS_LOADEDtests look solid (load/replace/empty/immutability).
- Always dispatch TASKS_LOADED even for empty arrays (clears stale tasks) - Add strict status validation in transformAPITask (defaults to 'pending') - Fix git config includeIf iteration (wildcards not supported) Resolves macroscopeapp and coderabbitai feedback on PR #243
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/opencode-review.yml (2)
1-103: Consider splitting infrastructure changes from feature work.The workflow enhancements (credential clearing, GitHub token usage, expanded prompts) appear unrelated to the PR's stated objective of fixing returning-user state loading (issue #231). While these infrastructure improvements are valuable, mixing them with feature work can complicate code review and make it harder to track changes.
Consider whether these workflow changes should be in a separate PR focused on CI/CD improvements.
47-61: Consider usingpersist-credentials: falseas a cleaner alternative to extensive credential clearing.The extensive credential clearing at lines 47-61 is reasonable given the default
persist-credentials: trueinactions/checkout@v4. However, a simpler approach would be to setpersist-credentials: falsein the checkout action (if credentials are not needed for subsequent steps), which would prevent the GitHub token from being added to git credentials in the first place.If credential clearing is necessary, note that the global
git configmodifications in GitHub Actions are scoped to the ephemeral job container and do not persist beyond the workflow execution, so the current approach at line 61 is safe. That said, if you only need to clear credentials within this job, using--localscope would be more explicit. Alternatively, consider whether all the credential clearing operations (lines 48-59) are necessary—you may only need to unsetcredential.helper.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/opencode-review.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
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
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
⏰ 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)
.github/workflows/opencode-review.yml (3)
40-40: Good security practice: disabling credential persistence.Setting
persist-credentials: falseprevents the checkout action from persisting credentials in the local git config, which aligns well with the enhanced credential clearing in the next step.
78-78: LGTM: GitHub token usage aligns with credential clearing.Adding
use_github_token: trueis appropriate given the credential clearing in the previous step, ensuring the OpenCode action can still authenticate properly.
79-101: Improved review guidance with comprehensive coverage.The expanded prompt provides clearer structure with specific review areas (code quality, bugs, performance, security, test coverage) and helpful notes about maintaining consistency with prior reviews. This will lead to more thorough and consistent PR reviews.
✅ Follow-up Review: Excellent Progress!SummaryThis PR successfully fixes the critical returning user UX issue where tasks were not visible without WebSocket events. The implementation is clean, well-tested, and addresses all major concerns from the previous review. Changes Since Last Review✅ RESOLVED Issues:
Code Quality AssessmentStrengths:
Technical Implementation:
Remaining Minor Observations1. Type Safety Enhancement Opportunity (Low Priority) 2. Test Assertion Gap (Documentation Issue) Security & Performance
Final VerdictStatus: APPROVED ✅ This is a well-executed fix that:
The remaining observations are minor type safety/documentation improvements that do not block merge. Recommendation: Ready to merge! 🚀 Great work addressing the previous review feedback - the unit tests for TASKS_LOADED, duplicate code removal, and CI fix demonstrate excellent follow-through. Review based on commit f30be68 |
The action requires GITHUB_TOKEN as an environment variable when use_github_token is set to true. Without it, the action fails with: "GITHUB_TOKEN environment variable is not set"
✅ Follow-up Review: All Recommendations Addressed!SummaryExcellent work addressing all the feedback from the previous review! The PR is now in great shape. ✅ Changes Since Last Review1. CI Workflow Fix (commits f30be68, 0f97623, 26e1a7b)
2. Unit Tests for TASKS_LOADED ✅ ADDRESSED
3. Duplicate Code Removed ✅ ADDRESSED
📋 Outstanding Items from Previous Review1. Test Assertion Clarity ℹ️ ACCEPTABLE AS-IS
2. TypeScript Type Safety ℹ️ RECOMMENDED (Low Priority)
3. Error Handling ℹ️ ACCEPTABLE AS-IS
🎯 Final VerdictStatus: ✅ APPROVED - Ready to Merge! Summary of Improvements:
Code Quality:
Minor Suggestion (optional, non-blocking): 🎉 Excellent Work!This PR demonstrates:
Recommendation: ✅ Merge when ready! Great job fixing issue #231! 🚀 |
|
Based on my comprehensive review of PR #243, here is my assessment: ✅ APPROVED - Ready to Merge!This PR successfully resolves issue #231 (returning user state reconciliation) with excellent implementation quality and comprehensive testing. Core Implementation QualityExcellent fix for critical UX issue:
Type Safety & Validation:
Test CoverageComprehensive E2E testing:
Unit testing:
DocumentationExcellent additions:
Minor Observations (Non-blocking)1. Potential Race Condition (documented trade-off)
2. Fixed Previous Issues ✅
Security & PerformanceNo concerns:
Code QualityFollows repository conventions:
RecommendationThis PR is production-ready. The fix is well-implemented, thoroughly tested, and all previous review feedback has been addressed. The distinction between returning users (no WebSocket events) and late-joining users (partial events) is clearly documented and properly validated. Great work! 🎉 Review based on latest commits (0f97623) |
Previously only dispatched when tasks was a valid array, leaving stale tasks when API returns data with null/undefined tasks field. Now dispatches whenever tasksData.data exists, defaulting to empty array.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/opencode-review.yml (2)
32-41: Pinactions/checkoutandanomalyco/opencode/githubto commit SHAs, not version tagsVersion tags are mutable and allow for supply-chain attacks. Pin both actions to full-length commit SHAs instead. The
persist-credentials: falsesetting is correct. Example:uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v6.1.0Similarly, replace
anomalyco/opencode/github@latestwith a pinned commit SHA. If you need to update pinned SHAs, use Dependabot or Renovate to automate detection and allow review before merging.Minor: Line 66 sets
credential.helperto empty string rather than unsetting it entirely—consider using--unsetinstead for consistency.
73-85: Pin to commit SHA and use correctgithub_tokeninput parameter
anomalyco/opencode/github@latestis non-deterministic and a supply-chain risk. Pin to a full commit SHA instead (e.g.,anomalyco/opencode/github@<commit-sha>).- The
use_github_tokeninput is not supported. Replace it with the correct input name:github_token: ${{ secrets.GITHUB_TOKEN }}in thewith:block.- Consider using
${{ github.token }}instead of${{ secrets.GITHUB_TOKEN }}for consistency with GitHub best practices.
🤖 Fix all issues with AI agents
In @.github/workflows/opencode-review.yml:
- Around line 42-67: The script misses includeIf keys due to case-sensitive grep
and dangerously sets credential.helper to an empty string; update the includeIf
cleanup to use case-insensitive matching (e.g., change grep "^includeif\." to
grep -i "^includeif\.") for both global and local loops so keys like
"includeIf." are removed, and remove the final line that does git config
--global credential.helper "" (rely on the existing --unset-all commands instead
of setting an empty helper).
In @web-ui/src/types/agentState.ts:
- Around line 167-182: transformAPITask is dropping dependency info: map
apiTask.depends_on (a comma-separated string) into the returned Task.blocked_by
number[]; parse the string by splitting on commas, trim each entry, convert to
integers (parseInt or Number), filter out empty strings and NaN values, and
default to an empty array when depends_on is null/empty; add the blocked_by
property to the returned object in transformAPITask so the UI receives the
dependency IDs.
- Line 161: The project has two conflicting TaskStatus definitions causing
silent conversion: update the canonical TaskStatus used by transformAPITask to
include 'assigned' and 'failed' (make agentState.ts match
web-ui/src/types/index.ts) and expand VALID_TASK_STATUSES to
['pending','in_progress','blocked','completed','assigned','failed'] (or import
the single shared TaskStatus/VALID list), then remove any unsafe "as TaskStatus"
casts in transformAPITask so statuses from the API are validated/preserved and
allowed through to TaskTreeView.tsx without being coerced to 'pending'.
🧹 Nitpick comments (1)
web-ui/src/types/agentState.ts (1)
144-156: Consider validating optional fields when present.The type guard only validates required fields. If optional fields like
progressortimestampare present but have invalid types (e.g.,progress: "invalid"), the validation passes buttransformAPITaskmay produce unexpected results.♻️ Optional enhancement: Validate optional fields
export function isValidTaskResponse(task: unknown): task is APITaskResponse { if (typeof task !== 'object' || task === null) return false; const t = task as Record<string, unknown>; - return ( + const hasRequiredFields = ( typeof t.id === 'number' && typeof t.project_id === 'number' && typeof t.title === 'string' && typeof t.status === 'string' ); + + if (!hasRequiredFields) return false; + + // Validate optional fields if present + if (t.progress !== undefined && typeof t.progress !== 'number') return false; + if (t.timestamp !== undefined && typeof t.timestamp !== 'number') return false; + if (t.priority !== undefined && typeof t.priority !== 'number') return false; + + return true; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/opencode-review.ymlweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
🧰 Additional context used
📓 Path-based instructions (4)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/AgentStateProvider.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/AgentStateProvider.tsx
web-ui/src/components/AgentStateProvider.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Files:
web-ui/src/components/AgentStateProvider.tsx
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
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
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/contexts/AgentStateContext.ts : Use context-based state management with React Context + useReducer pattern for Dashboard with AgentStateContext, agentReducer, and useAgentState hook
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
🧬 Code graph analysis (2)
web-ui/src/components/AgentStateProvider.tsx (2)
web-ui/src/types/agentState.ts (3)
Task(109-118)isValidTaskResponse(147-156)transformAPITask(167-182)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (1)
Task(97-105)
web-ui/src/types/agentState.ts (3)
web-ui/src/types/index.ts (1)
TaskStatus(7-7)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (2)
TaskStatus(43-47)Task(97-105)web-ui/src/types/api.ts (1)
Task(30-72)
⏰ 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). (5)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: opencode-review
- GitHub Check: claude-review
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (5)
web-ui/src/types/agentState.ts (3)
120-142: LGTM! Well-documented interface.The
APITaskResponseinterface clearly documents the API response structure with appropriate optional fields and helpful comments.
232-239: LGTM! Clean action interface.The
TasksLoadedActioninterface is well-documented and follows the established action pattern. The comment clearly explains its purpose for returning users.
364-364: LGTM! Action union updated correctly.The
AgentActionunion type correctly includes the newTasksLoadedAction.web-ui/src/components/AgentStateProvider.tsx (2)
22-23: LGTM! Imports are correct.The new imports from
@/types/agentStateare properly used in the task loading logic below.
120-149: Implementation looks solid with good defensive programming.The task loading logic correctly:
- Validates tasks with
isValidTaskResponsebefore transformation- Warns about invalid tasks in console
- Dispatches
TASKS_LOADEDeven for empty arrays to clear stale state- Includes comprehensive comments explaining the returning user scenario
The verbose cast on line 140 is acceptable for type safety. However, note that this code inherits the
depends_onmapping issue fromtransformAPITask(flagged in agentState.ts).Based on learnings, this aligns with the Context + Reducer pattern for multi-agent state management.
Review Summary: APPROVED ✅This PR successfully resolves the returning user UX issue by loading tasks from the API when WebSocket events are missed. All previous review concerns have been addressed. Core Implementation✅ Strong:
✅ Type Safety:
Test Coverage✅ Comprehensive:
Known Trade-off (Documented)
Security & Performance✅ No concerns:
RecommendationLGTM - Ready to Merge! 🚀 The fix is well-implemented, thoroughly tested, and all previous review feedback has been addressed. The distinction between returning users (no WebSocket events) and late-joining users (partial events) is clearly documented and properly validated. |
|
Review comment posted successfully. The PR addresses issue #231 with well-implemented API-first task loading, comprehensive test coverage, and all previous feedback addressed. Ready to merge. |
✅ Final Review - Ready to Merge!Review SummaryI've reviewed PR #243 across multiple iterations and can confirm this is excellent work that fully addresses issue #231. ✅ All Previous Feedback AddressedBased on my previous reviews, the following items have been successfully addressed:
📊 Code Quality AssessmentArchitecture ✅
Testing ✅ Outstanding!
Performance ✅
Security ✅
Maintainability ✅
🎯 What This PR FixesProblem: Users who navigated to a project after missing WebSocket events saw empty task lists because Solution:
Impact: Returning users now see correct project state immediately upon navigation, without depending on WebSocket events. 💡 Optional Enhancement (Non-Blocking)The only minor suggestion from previous reviews that remains is using the existing Current code (lines 136-144): const validTasks: Task[] = rawTasks
.filter((task: unknown) => {
if (!isValidTaskResponse(task)) {
console.warn('Invalid task response skipped:', task);
return false;
}
return true;
})
.map((task: unknown) => transformAPITask(task as Parameters<typeof transformAPITask>[0]));This is defensive and safe - the validation catches malformed data and logs warnings. No change required. 🚀 RecommendationStatus: ✅ APPROVED - Merge When Ready This PR demonstrates:
Confidence Level: High - All critical paths tested, no security/performance concerns identified. 📝 Closes✅ Fixes #231 - E2E test failures for returning user state reconciliation Great work on this fix! The distinction between returning users (no WebSocket events) and late-joining users (partial events) is now clearly documented and tested. 🎉 |
✅ APPROVED - Ready to MergeThe PR successfully resolves issue #231 (returning user state reconciliation) with excellent implementation quality and comprehensive test coverage. Core Implementation QualityExcellent fix for critical UX issue:
Type Safety & Validation:
Test CoverageComprehensive unit tests:
Comprehensive E2E testing:
Addressed Issues from Previous ReviewsAll major concerns from prior reviews have been resolved:
Outstanding Observations (Non-blocking)1. Type Inconsistency - TaskStatus Definitions ℹ️ Future ImprovementThere are two different
The 2. Missing depends_on Mapping ℹ️ Future ImprovementThe 3. Git Config Cleanup ℹ️ MinorThe CI workflow's includeIf cleanup uses case-sensitive grep ( Security & PerformanceNo concerns:
Known Trade-off (Documented & Acceptable)Race Condition: RecommendationThis PR is production-ready. The fix is well-implemented, thoroughly tested, and addresses the core issue. The outstanding observations are minor type safety and feature completeness improvements that can be addressed in follow-up PRs. Great work on the comprehensive testing and clear documentation! 🚀 Review based on commits 0f97623 and subsequent fixes |
|
Review comment posted successfully. The PR addresses issue #231 with a well-implemented API-first task loading solution, comprehensive E2E and unit test coverage, and all previous review feedback addressed. Summary:
Verdict: Approved and ready to merge. |
Workflow: - Use case-insensitive grep for includeIf keys (catches "includeIf") - Remove dangerous `credential.helper ""` (rely on --unset-all instead) Types: - Sync TaskStatus with index.ts: add 'assigned' and 'failed' statuses - Parse apiTask.depends_on string into blocked_by number array - Update TaskList filter options and styling for new statuses
Review Summary: APPROVED ✅This PR successfully resolves issue #231 (returning user state reconciliation) with excellent implementation quality and comprehensive test coverage. Core Implementation QualityExcellent fix for critical UX issue:
Type Safety & Validation:
Test CoverageComprehensive unit tests:
Comprehensive E2E testing:
DocumentationExcellent additions:
Addressed Issues from Previous ReviewsAll major concerns from prior reviews have been resolved:
Security & PerformanceNo concerns:
Known Trade-off (Documented & Acceptable)Race Condition: replaces all tasks: . If WebSocket events arrive before SWR completes, they could be overwritten. However, this is acceptable for the returning user use case where users explicitly need API state and WebSocket events are stale. The SWR configuration minimizes this risk, and the alternative (merging) could introduce stale WebSocket data for returning users. Code QualityFollows repository conventions:
RecommendationThis PR is production-ready. The fix is well-implemented, thoroughly tested, and addresses the core issue. All previously identified concerns have been resolved in this iteration. The distinction between returning users (no WebSocket events) and late-joining users (partial events) is clearly documented and properly validated. Great work on the comprehensive testing and clear documentation! 🚀 |
Review Summary: APPROVED ✅This PR successfully resolves issue #231 (returning user state reconciliation) with excellent implementation quality and comprehensive test coverage. Core Implementation QualityExcellent fix for critical UX issue:
Type Safety & Validation:
Test CoverageComprehensive unit tests:
Comprehensive E2E testing:
DocumentationExcellent additions:
Addressed Issues from Previous ReviewsAll major concerns from prior reviews have been resolved:
Security & PerformanceNo concerns:
Known Trade-off (Documented & Acceptable)Race Condition: TASKS_LOADED replaces all tasks. If WebSocket events arrive before SWR completes, they could be overwritten. This is acceptable for the returning user use case where users explicitly need API state and WebSocket events are stale. RecommendationThis PR is production-ready. The fix is well-implemented, thoroughly tested, and addresses the core issue. All previously identified concerns have been resolved. Great work on the comprehensive testing and clear documentation! |
Review Complete ✅I've reviewed PR #243 and posted an approved review comment on the pull request. SummaryThis PR successfully resolves issue #231 by adding API-first task loading for returning users. The implementation is excellent with comprehensive test coverage. Key Strengths:
Verdict: ✅ APPROVED - Ready to merge! |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/opencode-review.yml (1)
72-83: Pin action version and use${{ github.token }}for GITHUB_TOKEN.
@latestis non-deterministic and poses a supply-chain risk; pin to a specific semver tag (e.g.,v1.2.3) or full commit SHA.${{ github.token }}is the documented context for the automatically created token and is preferred over${{ secrets.GITHUB_TOKEN }}.Proposed change
- uses: anomalyco/opencode/github@latest + uses: anomalyco/opencode/github@v<VERSION_OR_SHA> env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_TOKEN: ${{ github.token }} ZHIPU_API_KEY: ${{ secrets.ZHIPU_API_KEY }}
🤖 Fix all issues with AI agents
In @.github/workflows/claude-code-review.yml:
- Line 15: Replace the hard-coded "if: false" constant with a repo-configurable
boolean variable so the job is disabled by default but can be toggled without
editing the workflow; e.g., change the condition to reference a
repository/organization variable like OPENCODE_REVIEW_ENABLED (use the
expression syntax: ${{ vars.OPENCODE_REVIEW_ENABLED == 'true' }} or similar) and
document or set the default variable to "false" in repo settings so the job
stays off until the variable is flipped.
🧹 Nitpick comments (4)
.github/workflows/opencode-review.yml (2)
42-65: Harden theincludeIfcleanup loops (avoid word-splitting) and use--unset-all.
Currentfor key in $(...)can mis-handle unusual keys;--unset-allis safer if a key has multiple values.Proposed change
- for key in $(git config --global --list --name-only 2>/dev/null | grep -i "^includeif\." || true); do - git config --global --unset "$key" || true - done - for key in $(git config --local --list --name-only 2>/dev/null | grep -i "^includeif\." || true); do - git config --local --unset "$key" || true - done + git config --global --list --name-only 2>/dev/null | grep -i "^includeif\." | while IFS= read -r key; do + git config --global --unset-all "$key" || true + done || true + git config --local --list --name-only 2>/dev/null | grep -i "^includeif\." | while IFS= read -r key; do + git config --local --unset-all "$key" || true + done || true
32-41: actions/checkout@v6 is valid.
actions/checkout@v6is a legitimate tag with v6.0.1 as the latest stable release (Dec 2, 2025). For enhanced supply-chain security, consider pinning to a commit SHA (e.g.,actions/checkout@<commit-sha>) instead of relying on major version tags, though major version pinning is a standard and acceptable practice.web-ui/src/components/AgentStateProvider.tsx (1)
136-144: Strengthen type safety by avoiding the Parameters utility type cast.Line 144 uses a complex cast
as Parameters<typeof transformAPITask>[0]to satisfy TypeScript. This pattern bypasses type safety because:
- The filter on line 137 already narrows
tasktoAPITaskResponsevia theisValidTaskResponsetype guard- The cast suggests the types aren't flowing correctly through the chain
♻️ Refactor to leverage type narrowing without casts
- const validTasks: Task[] = rawTasks - .filter((task: unknown) => { - if (!isValidTaskResponse(task)) { - console.warn('Invalid task response skipped:', task); - return false; - } - return true; - }) - .map((task: unknown) => transformAPITask(task as Parameters<typeof transformAPITask>[0])); + const validTasks: Task[] = rawTasks + .filter(isValidTaskResponse) + .map(transformAPITask);This works because:
isValidTaskResponseis a type guard, sofilter(isValidTaskResponse)narrows the array toAPITaskResponse[]- No cast needed in the map - TypeScript knows
taskisAPITaskResponse- Cleaner and safer
web-ui/src/types/agentState.ts (1)
165-172: Consider exporting VALID_TASK_STATUSES for reusability.While the constant is used internally by
transformAPITask, having a centralized list of valid statuses could be useful for other validation scenarios (e.g., in tests or API request builders).If you anticipate needing this list elsewhere, export it:
-const VALID_TASK_STATUSES: readonly TaskStatus[] = [ +export const VALID_TASK_STATUSES: readonly TaskStatus[] = [Otherwise, keeping it private is fine.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/claude-code-review.yml.github/workflows/opencode-review.ymlweb-ui/src/components/AgentStateProvider.tsxweb-ui/src/components/TaskList.tsxweb-ui/src/types/agentState.ts
🧰 Additional context used
📓 Path-based instructions (4)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/components/TaskList.tsxweb-ui/src/types/agentState.ts
web-ui/src/components/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.tsx: Use shadcn/ui Nova template components with semantic color palette (bg-card, text-foreground, etc.) and avoid hardcoded color values
Use cn() utility for conditional Tailwind CSS classes and follow Nova's compact spacing conventions
Files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/components/TaskList.tsx
web-ui/src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Replace all icon usage with Hugeicons (@hugeicons/react) and do not mix with lucide-react
Files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/components/TaskList.tsx
web-ui/src/components/AgentStateProvider.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Files:
web-ui/src/components/AgentStateProvider.tsx
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to tests/e2e/**/*.ts : Implement E2E tests using Playwright + TestSprite with loginUser() helper from tests/e2e/test-utils.ts for authentication
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/contexts/AgentStateContext.ts : Use context-based state management with React Context + useReducer pattern for Dashboard with AgentStateContext, agentReducer, and useAgentState hook
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/reducers/agentReducer.ts : Use Context + Reducer pattern for multi-agent support handling up to 10 concurrent agents with independent state tracking and timestamp conflict resolution using last-write-wins
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/AgentStateProvider.tsx : Wrap AgentStateProvider with ErrorBoundary component for graceful error handling in Dashboard
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/lib/websocketMessageMapper.ts : Implement WebSocket event mapping with 9 event types (agent_created, task_assigned, etc.) with automatic reconnection using exponential backoff (1s → 30s)
Applied to files:
web-ui/src/components/AgentStateProvider.tsxweb-ui/src/types/agentState.ts
📚 Learning: 2025-11-25T19:08:37.203Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Applies to docs/web-ui/src/**/*.{ts,tsx} : Use SWR for server state management and useState for local state in React
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/app/page.tsx : Implement automatic project discovery start after project creation with loading state transitions and 'Start Discovery' button for idle projects
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/components/Dashboard.tsx : Use React.memo on all Dashboard sub-components and useMemo for derived state to optimize performance with multi-agent support
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development with React 18, Next.js 14, Tailwind CSS, and Hugeicons for icons
Applied to files:
web-ui/src/components/AgentStateProvider.tsx
📚 Learning: 2026-01-04T06:26:12.870Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-04T06:26:12.870Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript strict mode and ensure 100% type safety with no hardcoded types or any usage in frontend code
Applied to files:
web-ui/src/types/agentState.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:
web-ui/src/types/agentState.ts
🧬 Code graph analysis (2)
web-ui/src/components/AgentStateProvider.tsx (2)
web-ui/src/types/agentState.ts (3)
Task(112-121)isValidTaskResponse(150-159)transformAPITask(194-213)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (1)
Task(97-105)
web-ui/src/types/agentState.ts (3)
web-ui/src/types/index.ts (1)
TaskStatus(7-7)specs/005-project-schema-refactoring/contracts/agent-state-api.ts (2)
TaskStatus(43-47)Task(97-105)web-ui/src/types/api.ts (1)
Task(30-72)
🪛 actionlint (1.7.10)
.github/workflows/claude-code-review.yml
15-15: constant expression "false" in condition. remove the if: section
(if-cond)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Backend Unit Tests
- GitHub Check: Frontend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
🔇 Additional comments (12)
web-ui/src/components/TaskList.tsx (4)
32-36: LGTM! New filter options align with expanded task statuses.The addition of 'Assigned' and 'Failed' filters properly extends the UI to support the new task statuses introduced in the type definitions.
50-53: LGTM! Status styling for new statuses is consistent.The visual treatment for 'failed' and 'assigned' statuses follows the established pattern and uses appropriate semantic colors from the design system.
90-90: LGTM! Test attribute enables E2E validation.The
data-statusattribute directly supports the E2E test scenarios described in the PR objectives for verifying returning-user state reconciliation.
192-196: LGTM! Filter counts initialization is complete.All task statuses from the expanded
TaskStatustype are now accounted for in the counts object.web-ui/src/components/AgentStateProvider.tsx (2)
122-128: LGTM! Documentation clearly explains the critical fix.The comment effectively communicates the root cause from issue #231 and why this change is essential for returning users.
130-134: The code is correctly handling task updates across project navigation and the empty array dispatch is necessary.When a user navigates between projects (e.g.,
/projects/1→/projects/2), thetasksDataSWR query automatically refetches because its key includes theprojectId. TheuseEffectat line 129 depends ontasksDataand therefore runs whenever the API returns new data for a different project. If the new project has no tasks, dispatching an empty array correctly clears stale tasks from the previous project in the reducer state.This is not redundant on initial mount—it's essential for the remounting scenario you identified. Since
AgentStateProvideris not keyed byprojectId, it persists across project navigation rather than remounting, making this dispatch necessary to keep reducer state in sync with API responses across different projects.Likely an incorrect or invalid review comment.
web-ui/src/types/agentState.ts (6)
42-50: LGTM! TaskStatus expansion properly documented.The addition of 'assigned' and 'failed' statuses extends the state machine appropriately, and the comment on line 42 correctly references the requirement to stay in sync with
web-ui/src/types/index.ts.
123-145: LGTM! APITaskResponse interface comprehensively models backend data.The interface correctly distinguishes API field names (
assigned_to,depends_on) from internal field names (agent_id,blocked_by), and the optional fields account for real-world API responses where not all fields are guaranteed.
150-159: LGTM! Validation logic is minimal and correct.The type guard checks only the required fields (
id,project_id,title,status), which aligns with the transformation function's requirements.
194-213: LGTM! Task transformation logic is defensive and correct.The function:
- Safely defaults invalid statuses to 'pending' (line 196-198)
- Correctly parses
depends_onintoblocked_by(line 201)- Maps API field names to internal names (
assigned_to→agent_id)- Provides a timestamp fallback (line 211)
This defensive approach prevents invalid API data from breaking the UI state.
263-270: LGTM! TasksLoadedAction properly typed.The action follows the established pattern and enables the reducer to handle bulk task initialization from the API.
178-188: The function appropriately remains unexported.Confirmed that
parseDependsOnis only called by the exportedtransformAPITaskat line 201 and there are no other places in the codebase parsing comma-separated task IDs. The current encapsulation is appropriate.
|
|
||
| jobs: | ||
| claude-review: | ||
| if: false # Disabled - using opencode-review.yml instead |
There was a problem hiding this comment.
Avoid if: false (breaks actionlint / permanently disables the job with no easy toggle).
Since actionlint flags constant-false conditions, prefer a repo variable toggle so it’s disabled by default but can be re-enabled without changing the workflow.
Proposed change
jobs:
claude-review:
- if: false # Disabled - using opencode-review.yml instead
+ # Disabled by default; set repo variable ENABLE_CLAUDE_REVIEW=true to enable.
+ if: ${{ vars.ENABLE_CLAUDE_REVIEW == 'true' }}
runs-on: ubuntu-latest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if: false # Disabled - using opencode-review.yml instead | |
| claude-review: | |
| # Disabled by default; set repo variable ENABLE_CLAUDE_REVIEW=true to enable. | |
| if: ${{ vars.ENABLE_CLAUDE_REVIEW == 'true' }} | |
| runs-on: ubuntu-latest |
🧰 Tools
🪛 actionlint (1.7.10)
15-15: constant expression "false" in condition. remove the if: section
(if-cond)
🤖 Prompt for AI Agents
In @.github/workflows/claude-code-review.yml at line 15, Replace the hard-coded
"if: false" constant with a repo-configurable boolean variable so the job is
disabled by default but can be toggled without editing the workflow; e.g.,
change the condition to reference a repository/organization variable like
OPENCODE_REVIEW_ENABLED (use the expression syntax: ${{
vars.OPENCODE_REVIEW_ENABLED == 'true' }} or similar) and document or set the
default variable to "false" in repo settings so the job stays off until the
variable is flipped.
1 participant
Summary
AgentStateProviderdidn't load tasks from API - only via WebSocket eventsTASKS_LOADEDaction to load tasks when SWR fetches themChanges
agentState.tsTasksLoadedActiontypeagentReducer.tsTASKS_LOADEDreducer caseAgentStateProvider.tsxTASKS_LOADEDwhen tasks arrive from SWRTaskList.tsxdata-statusattribute for E2E testingDashboard.tsxdata-testidanddata-phaseto status badgetest_returning_user.spec.tstest-utils.tsREADME.mdTest plan
Closes #231
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.