feat(ui): add PR management UI components and Dashboard integration #285
Conversation
This commit implements a complete pull request management interface: Components: - PRList: Displays PRs with status badges, filtering, CI/review status - PRCreationDialog: Form dialog for creating new PRs - PRMergeDialog: Confirmation dialog with merge method selection Infrastructure: - Added PR types (PullRequest, PRStatus, MergeMethod, etc.) - Added pullRequestsApi client methods (list, create, merge, close) - Added DashboardTab type for 'pull-requests' - Added shadcn/ui components: Textarea, Label, Checkbox, RadioGroup Dashboard Integration: - New "Pull Requests" tab between Metrics and Context - WebSocket integration for real-time PR status updates - PR creation and merge dialogs wired to Dashboard state Tests: - PRList.test.tsx: 21 tests covering rendering, filtering, actions - PRCreationDialog.test.tsx: Form validation and submission tests - PRMergeDialog.test.tsx: Merge method selection and confirmation tests Dependencies: - @radix-ui/react-label, @radix-ui/react-checkbox, @radix-ui/react-radio-group
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a Pull Request feature to the web UI: new PRList, PRCreationDialog, PRMergeDialog components and types; pullRequestsApi client; Radix-based UI primitives; Dashboard tab integration; comprehensive tests; and standardized test-time mocks for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as PRCreationDialog
participant API as pullRequestsApi (HTTP)
participant Server
participant WS as WebSocket (PRList)
User->>UI: submit create PR (branch,title,body)
UI->>API: POST /api/projects/{id}/prs (CreatePRRequest)
API->>Server: forward request
Server-->>API: created PR response
API-->>UI: CreatePRResponse
Server-->>WS: broadcast pr_created event
WS-->>UI: PRList receives event -> SWR mutate -> refresh list
sequenceDiagram
participant User
participant UI as PRMergeDialog
participant API as pullRequestsApi (HTTP)
participant Server
participant WS as WebSocket (PRList)
User->>UI: confirm merge (method, delete_branch)
UI->>API: POST /api/projects/{id}/prs/{prNumber}/merge (MergePRRequest)
API->>Server: forward merge request
Server-->>API: merge result
API-->>UI: MergePRResponse
Server-->>WS: broadcast pr_merged/pr_closed
WS-->>UI: PRList receives event -> SWR mutate -> update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🧰 Additional context used📓 Path-based instructions (2)web-ui/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
web-ui/src/components/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (12)📓 Common learnings📚 Learning: 2025-11-25T19:08:37.203ZApplied to files:
📚 Learning: 2025-11-25T19:08:37.203ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
📚 Learning: 2025-11-25T19:08:37.203ZApplied to files:
📚 Learning: 2026-01-11T23:33:31.895ZApplied to files:
🧬 Code graph analysis (2)web-ui/src/components/pr/PRList.tsx (3)
web-ui/src/components/Dashboard.tsx (4)
⏰ 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)
🔇 Additional comments (15)
✏️ Tip: You can disable this entire section by setting Comment |
Add Pull Requests tab to
|
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Code Review SummaryThis PR adds a comprehensive pull request management interface with excellent overall structure, type safety, and test coverage. The implementation follows project conventions (Nova theme, Hugeicons, SWR, TypeScript). Below are my findings: ✅ Strengths
|
| Component | Tests | Coverage Notes |
|---|---|---|
| PRList | 21 | ✅ Rendering, filtering, actions, states |
| PRCreationDialog | 13 | ✅ Form validation, submission, error handling |
| PRMergeDialog | 13 | ✅ Merge methods, validation, submission |
All tests are well-structured and follow TDD principles. Consider adding integration tests for the full PR workflow (create → list → merge).
🔐 Security
No security concerns identified. All API calls go through the authenticated client (uses from localStorage). The only external links are GitHub PR URLs with .
📈 Performance
- Good use of on subcomponents
- SWR caching and 30s refresh interval are appropriate
- Consider debouncing the filter change if filtering is done locally
✅ Conclusion
The PR implements a well-architected PR management interface with excellent type safety and test coverage. The main issues to address are:
- Remove the type in the WebSocket handler
- Implement or remove the incomplete PR view action
- Decide on single source of truth for filtering (server vs client)
These changes would make the implementation production-ready. The overall code quality is high and follows all project conventions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@web-ui/src/components/pr/PRList.tsx`:
- Around line 330-340: The SWR key used in useSWR doesn't include the filter,
causing stale responses when filter changes; update the key from
`/api/projects/${projectId}/prs` to include filter (e.g.
`/api/projects/${projectId}/prs?filter=${filter}` or use an array key like
[`/api/projects/${projectId}/prs`, projectId, filter]) so SWR revalidates
per-filter, keep the existing fetcher that calls pullRequestsApi.list(projectId,
filter === 'all' ? undefined : filter). After this change remove the useEffect
that calls mutate() on filter changes (the effect that references mutate) since
SWR will handle revalidation automatically.
In `@web-ui/src/components/pr/PRMergeDialog.tsx`:
- Around line 76-89: PRMergeDialog currently retains previous form and error
state across opens; add a useEffect that watches isOpen and when it becomes true
resets mergeMethod via setMergeMethod('squash'), resets deleteBranch via
setDeleteBranch(true), clears error via setError(null) and ensures isSubmitting
is false via setIsSubmitting(false) so the dialog always opens with a clean
state; place this effect inside the PRMergeDialog component near the other
useState calls and reference the existing state setters (setMergeMethod,
setDeleteBranch, setError, setIsSubmitting).
In `@web-ui/src/types/pullRequest.ts`:
- Around line 212-217: PR_STATUS_COLORS uses hardcoded Tailwind classes; replace
them with Nova design system variables: update the PR_STATUS_COLORS record (type
PRStatus) so the entries map as follows — open: { bg: 'bg-success/10', text:
'text-success' }; merged: { bg: 'bg-primary/10', text: 'text-primary' } (or use
secondary pair per design: bg-secondary and text-secondary-foreground); closed:
{ bg: 'bg-destructive/10', text: 'text-destructive' }; draft: { bg: 'bg-muted',
text: 'text-muted-foreground' }; keep the same object shape and export name
PR_STATUS_COLORS so consumers using PRStatus still work.
🧹 Nitpick comments (11)
web-ui/src/components/ui/checkbox.tsx (1)
24-24: UseCheckmarkIconinstead ofCheckmarkSquare01Iconto avoid visual redundancy.The
CheckmarkSquare01Iconrenders a square with a checkmark, but since the checkbox container is already styled as a square (rounded-sm), this creates unnecessary visual duplication. A simple checkmark icon would be more appropriate for the indicator.♻️ Suggested change
-import { CheckmarkSquare01Icon } from "@hugeicons/react" +import { CheckmarkIcon } from "@hugeicons/react"- <CheckmarkSquare01Icon className="h-3 w-3" /> + <CheckmarkIcon className="h-3 w-3" />web-ui/__tests__/components/PRCreationDialog.test.tsx (2)
24-37: Unused mock may be dead code.The
gitApi.getBranchesmock is defined but none of the tests appear to exercise it. Looking at the component implementation in the relevant code snippet,PRCreationDialoguses a staticSelectwith hardcoded branch options (main,develop,staging) rather than fetching branches dynamically.Consider removing this mock if it's not needed, or verify if there's a planned feature to fetch branches dynamically.
🧹 Suggested cleanup
-// Mock git API for branch list -jest.mock('@/api/git', () => ({ - gitApi: { - getBranches: jest.fn().mockResolvedValue({ - data: { - branches: [ - { id: 1, branch_name: 'feature/auth', status: 'active' }, - { id: 2, branch_name: 'feature/dashboard', status: 'active' }, - { id: 3, branch_name: 'main', status: 'active' }, - ], - }, - }), - }, -}));
99-104: Test name doesn't match assertion.The test is named "should have base branch selector with main as default" but only verifies the element exists without checking that "main" is the default value.
💡 Consider verifying the default value
it('should have base branch selector with main as default', () => { render(<PRCreationDialog {...defaultProps} />); const baseBranchSelect = screen.getByLabelText(/target branch/i); expect(baseBranchSelect).toBeInTheDocument(); + // Verify default value is displayed + expect(screen.getByText('main')).toBeInTheDocument(); });web-ui/__tests__/components/PRMergeDialog.test.tsx (1)
82-107: Consider adding a form reset test.The
PRCreationDialogtests include a "should reset form when reopened" test case. For consistency and to verify merge options reset between uses (e.g., if user selects "rebase" then closes and reopens), consider adding a similar test here.💡 Example reset test
it('should reset to default options when reopened', async () => { const user = userEvent.setup(); const { rerender } = render(<PRMergeDialog {...defaultProps} />); // Change merge method const rebaseRadio = screen.getByLabelText(/rebase and merge/i); await user.click(rebaseRadio); expect(rebaseRadio).toBeChecked(); // Close and reopen rerender(<PRMergeDialog {...defaultProps} isOpen={false} />); rerender(<PRMergeDialog {...defaultProps} isOpen={true} />); // Should have squash selected again const squashRadio = screen.getByLabelText(/squash and merge/i); expect(squashRadio).toBeChecked(); });web-ui/src/components/Dashboard.tsx (3)
116-120: Remove unused state or implement the feature.
_selectedPRNumberis declared but never used except to be set. The underscore prefix and eslint-disable suggest this is intentional dead code for future use. Either implement the PR detail view navigation now or remove the state entirely to avoid confusion.♻️ Suggested removal if not needed yet
// Pull Request management state (Feature: PR Management UI) const [showPRCreationDialog, setShowPRCreationDialog] = useState(false); const [prToMerge, setPrToMerge] = useState<PullRequest | null>(null); - // eslint-disable-next-line `@typescript-eslint/no-unused-vars` - const [_selectedPRNumber, setSelectedPRNumber] = useState<number | null>(null);
901-904: TODO indicates incomplete feature – consider tracking.The
onViewPRhandler sets state but the TODO comment indicates navigation is not yet implemented. This could confuse users who click "View" expecting something to happen.Would you like me to open an issue to track implementing the PR detail view navigation?
1000-1010: Redundant null check in conditional rendering.The
isOpen={prToMerge !== null}check is redundant since the component is already inside a{prToMerge && ...}block. While not incorrect, it's unnecessary.♻️ Simplified condition
{prToMerge && ( <PRMergeDialog pr={prToMerge} projectId={projectId} - isOpen={prToMerge !== null} + isOpen={true} onClose={() => setPrToMerge(null)} onSuccess={() => { setPrToMerge(null); // The PRList will auto-refresh via WebSocket }} /> )}web-ui/src/components/pr/PRCreationDialog.tsx (1)
104-118: Consider adding branch name format validation.The current validation only checks for empty strings. Git branch names have specific format requirements (e.g., no spaces, no
.., cannot end with.lock). Consider adding basic format validation to catch common mistakes early.web-ui/src/components/pr/PRMergeDialog.tsx (1)
54-70: Duplicate MERGE_METHODS definition – consider importing from types.This constant duplicates the one defined in
web-ui/src/types/pullRequest.ts(lines 221-234). Keeping two copies risks them diverging over time.♻️ Import from types instead
-const MERGE_METHODS: { value: MergeMethod; label: string; description: string }[] = [ - { - value: 'squash', - label: 'Squash and merge', - description: 'Combine all commits into one before merging', - }, - { - value: 'merge', - label: 'Create merge commit', - description: 'Preserve all commits with a merge commit', - }, - { - value: 'rebase', - label: 'Rebase and merge', - description: 'Reapply commits on top of base branch', - }, -]; +import { MERGE_METHODS } from '@/types/pullRequest'; + +// Transform to array format for mapping +const MERGE_METHOD_OPTIONS = (Object.entries(MERGE_METHODS) as [MergeMethod, { label: string; description: string }][]).map( + ([value, config]) => ({ value, ...config }) +);Then use
MERGE_METHOD_OPTIONSin the RadioGroup mapping.web-ui/src/components/pr/PRList.tsx (2)
375-383: Redundant local filtering after server-side filter.The API is already filtering by status (when filter !== 'all'), then the same filter is applied locally on line 381-383. If the SWR key issue is fixed, the local filter becomes fully redundant for filtered views.
If you intend to always fetch all PRs and filter locally for instant UI response, then remove the server-side filter parameter. Otherwise, remove the local filter.
321-326: Consider memoizing PRList for Dashboard performance.Per coding guidelines, Dashboard sub-components should use
React.memo. While the helper components are memoized, the mainPRListexport is not.♻️ Export memoized component
-export default function PRList({ +function PRList({ projectId, onCreatePR, onViewPR, onMergePR, }: PRListProps) { // ... component body } + +export default memo(PRList);Based on learnings, Dashboard sub-components should use
React.memofor performance optimization.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
web-ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
web-ui/__tests__/components/PRCreationDialog.test.tsxweb-ui/__tests__/components/PRList.test.tsxweb-ui/__tests__/components/PRMergeDialog.test.tsxweb-ui/package.jsonweb-ui/src/components/Dashboard.tsxweb-ui/src/components/pr/PRCreationDialog.tsxweb-ui/src/components/pr/PRList.tsxweb-ui/src/components/pr/PRMergeDialog.tsxweb-ui/src/components/pr/index.tsweb-ui/src/components/ui/checkbox.tsxweb-ui/src/components/ui/label.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/ui/textarea.tsxweb-ui/src/lib/api.tsweb-ui/src/types/dashboard.tsweb-ui/src/types/pullRequest.ts
🧰 Additional context used
📓 Path-based instructions (3)
web-ui/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/**/*.{ts,tsx}: Use TypeScript 5.3+ for frontend development
Use React 18 and Next.js 14 for frontend development
Use Tailwind CSS for styling with Nova design system template
Use shadcn/ui components from@/components/ui/for UI elements
Use Hugeicons (@hugeicons/react) for all icons, never mix with lucide-react
Use Nova color palette variables (bg-card, text-foreground, etc.) instead of hardcoded color values
Use cn() utility for conditional CSS classes in React components
Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
Include auth token as query parameter in WebSocket connections (?token=TOKEN)
Store auth tokens in localStorage with key 'auth_token' and include token in API requests via Authorization header
Files:
web-ui/src/components/ui/checkbox.tsxweb-ui/src/components/pr/index.tsweb-ui/src/components/ui/label.tsxweb-ui/src/types/dashboard.tsweb-ui/src/components/pr/PRList.tsxweb-ui/src/components/pr/PRCreationDialog.tsxweb-ui/src/components/pr/PRMergeDialog.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/lib/api.tsweb-ui/src/components/Dashboard.tsxweb-ui/src/components/ui/textarea.tsxweb-ui/src/types/pullRequest.ts
web-ui/src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
web-ui/src/components/**/*.{ts,tsx}: Use React Context with useReducer for centralized state management in Dashboard
Wrap AgentStateProvider with ErrorBoundary component for graceful error handling
Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Files:
web-ui/src/components/ui/checkbox.tsxweb-ui/src/components/pr/index.tsweb-ui/src/components/ui/label.tsxweb-ui/src/components/pr/PRList.tsxweb-ui/src/components/pr/PRCreationDialog.tsxweb-ui/src/components/pr/PRMergeDialog.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/Dashboard.tsxweb-ui/src/components/ui/textarea.tsx
web-ui/src/lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement WebSocket automatic reconnection with exponential backoff (1s → 30s) and full state resync
Files:
web-ui/src/lib/api.ts
🧠 Learnings (16)
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use shadcn/ui components from `@/components/ui/` for UI elements
Applied to files:
web-ui/package.jsonweb-ui/src/components/ui/checkbox.tsxweb-ui/src/components/pr/index.tsweb-ui/src/components/ui/label.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/Dashboard.tsxweb-ui/src/components/ui/textarea.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use React 18 and Next.js 14 for frontend development
Applied to files:
web-ui/package.jsonweb-ui/src/components/ui/textarea.tsx
📚 Learning: 2026-01-05T02:39:31.302Z
Learnt from: frankbria
Repo: frankbria/codeframe PR: 184
File: web-ui/package.json:48-48
Timestamp: 2026-01-05T02:39:31.302Z
Learning: The package `baseline-browser-mapping` in `web-ui/package.json` devDependencies is intentionally added to address Playwright warnings about outdated browser compatibility data, even though it's not directly imported in the codebase.
Applied to files:
web-ui/package.json
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use Hugeicons (hugeicons/react) for all icons, never mix with lucide-react
Applied to files:
web-ui/package.jsonweb-ui/src/components/ui/checkbox.tsxweb-ui/src/components/Dashboard.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/components/**/*.{ts,tsx} : Use functional React components with TypeScript interfaces
Applied to files:
web-ui/src/components/ui/checkbox.tsxweb-ui/src/components/pr/index.tsweb-ui/src/components/ui/label.tsxweb-ui/src/components/pr/PRList.tsxweb-ui/src/components/pr/PRMergeDialog.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/ui/textarea.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/components/**/*.{ts,tsx} : Use PascalCase for React component names
Applied to files:
web-ui/src/components/ui/checkbox.tsxweb-ui/src/components/ui/label.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/ui/textarea.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use cn() utility for conditional CSS classes in React components
Applied to files:
web-ui/src/components/ui/checkbox.tsxweb-ui/src/components/ui/label.tsxweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/ui/textarea.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use TypeScript 5.3+ for frontend development
Applied to files:
web-ui/src/components/ui/checkbox.tsxweb-ui/src/components/ui/textarea.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/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
web-ui/__tests__/components/PRList.test.tsxweb-ui/__tests__/components/PRCreationDialog.test.tsxweb-ui/__tests__/components/PRMergeDialog.test.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/**/*.{ts,tsx,js,jsx} : Use named exports instead of default exports in TypeScript/JavaScript
Applied to files:
web-ui/src/components/pr/index.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Use React Context with useReducer for centralized state management in Dashboard
Applied to files:
web-ui/src/types/dashboard.tsweb-ui/src/components/ui/radio-group.tsxweb-ui/src/components/Dashboard.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/pr/PRList.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 Tailwind utility classes for styling instead of CSS modules
Applied to files:
web-ui/src/components/ui/radio-group.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use process.env.NEXT_PUBLIC_API_URL with fallback to http://localhost:8080 for API endpoint configuration
Applied to files:
web-ui/src/lib/api.ts
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Applied to files:
web-ui/src/components/Dashboard.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/reducers/**/*.{ts,tsx} : Use useReducer with 13+ action types for agent state management; implement timestamp conflict resolution with last-write-wins strategy
Applied to files:
web-ui/src/components/Dashboard.tsx
🧬 Code graph analysis (10)
web-ui/src/components/ui/checkbox.tsx (2)
web-ui/__mocks__/@hugeicons/react.js (1)
React(6-6)web-ui/src/lib/utils.ts (1)
cn(4-6)
web-ui/__tests__/components/PRList.test.tsx (2)
web-ui/src/types/pullRequest.ts (1)
PullRequest(41-98)web-ui/src/components/pr/PRList.tsx (1)
PRList(321-458)
web-ui/src/components/ui/label.tsx (2)
web-ui/__mocks__/@hugeicons/react.js (1)
React(6-6)web-ui/src/lib/utils.ts (1)
cn(4-6)
web-ui/src/components/pr/PRCreationDialog.tsx (2)
web-ui/src/types/pullRequest.ts (1)
CreatePRRequest(107-119)web-ui/src/lib/api.ts (1)
pullRequestsApi(268-321)
web-ui/src/components/pr/PRMergeDialog.tsx (2)
web-ui/src/types/pullRequest.ts (3)
PullRequest(41-98)MERGE_METHODS(222-235)MergeMethod(22-22)web-ui/src/lib/api.ts (1)
pullRequestsApi(268-321)
web-ui/__tests__/components/PRCreationDialog.test.tsx (2)
web-ui/src/components/pr/PRCreationDialog.tsx (1)
PRCreationDialog(68-265)web-ui/src/lib/api.ts (1)
pullRequestsApi(268-321)
web-ui/src/lib/api.ts (1)
web-ui/src/types/pullRequest.ts (8)
PRStatus(17-17)PRListResponse(164-167)PullRequest(41-98)CreatePRRequest(107-119)CreatePRResponse(139-144)MergePRRequest(124-130)MergePRResponse(149-152)ClosePRResponse(157-159)
web-ui/src/components/Dashboard.tsx (4)
web-ui/src/types/pullRequest.ts (1)
PullRequest(41-98)web-ui/src/components/pr/PRList.tsx (1)
PRList(321-458)web-ui/src/components/pr/PRCreationDialog.tsx (1)
PRCreationDialog(68-265)web-ui/src/components/pr/PRMergeDialog.tsx (1)
PRMergeDialog(76-219)
web-ui/__tests__/components/PRMergeDialog.test.tsx (3)
web-ui/src/types/pullRequest.ts (1)
PullRequest(41-98)web-ui/src/components/pr/PRMergeDialog.tsx (1)
PRMergeDialog(76-219)web-ui/src/lib/api.ts (1)
pullRequestsApi(268-321)
web-ui/src/components/ui/textarea.tsx (2)
web-ui/__mocks__/@hugeicons/react.js (1)
React(6-6)web-ui/src/lib/utils.ts (1)
cn(4-6)
⏰ 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: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: Macroscope - Correctness Check
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: opencode-review
🔇 Additional comments (29)
web-ui/src/components/ui/textarea.tsx (1)
1-23: LGTM! Well-implemented Textarea component following shadcn/ui patterns.The component correctly uses
forwardRef, thecn()utility for class merging, Nova design system variables, and proper TypeScript typing. The implementation aligns with the project's component conventions.web-ui/src/components/ui/radio-group.tsx (1)
1-44: Component implementation follows shadcn/ui patterns correctly.The RadioGroup and RadioGroupItem components properly use
forwardRef, thecn()utility, Nova design system variables, and Hugeicons as per coding guidelines. The implementation is well-structured with appropriate focus and disabled state styling.web-ui/src/components/ui/checkbox.tsx (1)
1-30: Component structure follows shadcn/ui patterns correctly.The Checkbox component properly uses
forwardRef, thecn()utility, Radix primitives, and Nova design system variables. The implementation includes appropriate styling for focus states, disabled states, and checked state transitions.web-ui/__tests__/components/PRList.test.tsx (4)
101-115: Module-level mutable mock state requires careful test isolation.The module-level variables (
mockSWRData,mockSWRError,mockSWRIsLoading) are reset inbeforeEach, which is correct. However, if tests run in parallel or if any test modifies these after the SWR mock is invoked, there could be race conditions. The current implementation works but is worth noting for future maintainers.
19-32: Comprehensive Hugeicons mock setup.The mock covers all icons used in the PRList component with appropriate test IDs for assertions. This aligns with the project's guideline to use Hugeicons and prevents import issues in tests.
340-349: Good security validation for external links.The test correctly verifies that GitHub links include
target="_blank"andrel="noopener noreferrer"attributes, which prevents tabnabbing attacks. This is an important security check.
1-350: Well-structured test suite with comprehensive coverage.The test file follows TDD principles with clear documentation, covers all major component behaviors (rendering, filtering, actions, states, external links), and uses recommended testing patterns (
userEvent.setup(),within()for scoped queries). Test organization in describe blocks is logical and maintainable.web-ui/package.json (1)
17-22: LGTM! New Radix UI dependencies use current versions.The new dependencies for checkbox, label, and radio-group primitives are correctly versioned to the latest releases and placed in alphabetical order, consistent with existing Radix UI dependencies in the project.
web-ui/src/components/pr/index.ts (1)
1-8: LGTM!Clean barrel module that consolidates PR component exports, enabling cleaner imports elsewhere (e.g.,
import { PRList, PRCreationDialog } from '@/components/pr').web-ui/__tests__/components/PRCreationDialog.test.tsx (1)
129-183: Good test coverage for form submission flow.Tests comprehensively cover the submission lifecycle: API call verification with correct payload, success callback, dialog close, and loading state display.
web-ui/src/types/dashboard.ts (1)
6-6: LGTM!The
DashboardTabtype is correctly expanded to include'pull-requests', enabling type-safe tab switching for the new PR management feature.web-ui/src/components/ui/label.tsx (1)
1-26: LGTM!Standard shadcn/ui
Labelcomponent following established patterns:forwardReffor ref forwarding,cn()utility for class composition, and Radix primitive wrapping. Consistent with other UI primitives in the codebase. As per coding guidelines, this correctly uses shadcn/ui patterns with thecn()utility.web-ui/__tests__/components/PRMergeDialog.test.tsx (2)
11-31: Well-structured mock data.The
mockPRobject provides a completePullRequestinstance with all required fields, enabling comprehensive component testing. Good practice to define this at the module level for reuse across test cases.
128-179: Thorough form submission test coverage.Tests correctly verify:
- API called with correct project ID, PR number, and merge options
- Success callback invoked after merge
- Dialog closes on success
- Loading state displayed during submission
web-ui/src/components/Dashboard.tsx (2)
531-545: LGTM! Pull Requests tab follows established patterns.The new tab implementation is consistent with other tabs, uses Hugeicons as per coding guidelines, and properly sets up ARIA attributes for accessibility.
890-909: LGTM! Pull Requests panel integration is well-structured.The panel follows the same pattern as other tab panels with proper
role="tabpanel",data-testid, and consistent styling. Actions are properly wired to state handlers.web-ui/src/components/pr/PRCreationDialog.tsx (4)
68-76: LGTM! Component structure and props are well-designed.The component exports a clear interface with sensible defaults and follows TypeScript best practices.
90-102: Good form reset pattern on dialog open.Resetting form state when the dialog opens ensures a clean slate and prevents stale data from previous submissions.
191-207: Hardcoded target branch list may need future enhancement.The target branch options (main, develop, staging) are hardcoded. This works for common workflows but may not fit all projects. Consider fetching available branches from the API in a future iteration.
120-146: LGTM! Submission handler with proper error handling.The async handler correctly manages loading state, clears previous errors, and handles both success and failure cases appropriately.
web-ui/src/components/pr/PRMergeDialog.tsx (2)
91-109: LGTM! Merge handler with proper error handling.The async handler correctly manages submission state and provides meaningful error messages.
182-189: Good UX with irreversibility warning.The warning banner clearly communicates that merging is permanent and highlights the target branch. Good use of semantic colors for warning state.
web-ui/src/lib/api.ts (2)
9-18: LGTM! Type imports are complete and organized.All necessary PR types are imported for the API client methods.
260-321: LGTM! Well-structured PR API client.The
pullRequestsApiobject follows the existing patterns in the file with:
- Clear JSDoc documentation with parameter descriptions
- Correct HTTP verbs for each operation
- Proper TypeScript generics for response types
- Consistent endpoint structure
web-ui/src/components/pr/PRList.tsx (3)
85-112: LGTM! Well-designed CI status indicator.Good use of
memo(), proper fallback for unknown status, and accessibletitleattribute for tooltip.
155-241: LGTM! PRCard is well-structured with conditional rendering.The card properly handles optional fields, conditionally shows the Merge button for open PRs, and uses proper accessibility attributes on the GitHub link.
347-373: LGTM! WebSocket subscription with proper cleanup.The effect correctly subscribes to WebSocket messages, filters by project ID, and returns a cleanup function.
web-ui/src/types/pullRequest.ts (2)
255-260: Consider whethercanMergePRshould account for review status.Currently,
canMergePRonly checks that the PR is open and CI hasn't failed. Depending on your workflow requirements, you may want to also block merging whenreview_status === 'changes_requested'.If this is intentional (e.g., allowing force-merge regardless of review), the current implementation is fine. Otherwise, consider:
export function canMergePR(pr: PullRequest): boolean { - return pr.status === 'open' && pr.ci_status !== 'failure'; + return pr.status === 'open' && + pr.ci_status !== 'failure' && + pr.review_status !== 'changes_requested'; }
1-98: Well-structured type definitions.The type definitions are comprehensive, well-documented, and properly organized. The separation of status types, core entity types, API request/response types, and state management types provides clear boundaries for different concerns.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Add global @hugeicons/react mock to jest.setup.js with all 37 icons - Fix loading state tests in PRCreationDialog and PRMergeDialog by using deferred promises to keep loading visible during assertions - Update Dashboard.test.tsx with complete Hugeicons mock - Update integration tests with complete Hugeicons mocks - Fix PRMergeDialog test to handle multiple "main" text matches
Code Review SummaryThis PR implements a comprehensive PR management UI with excellent type safety and test coverage. The implementation follows project conventions well. Here are my observations: Strengths
Issues to Address1. Type Safety: WebSocket Handler (Medium)Location: PRList.tsx:360 The WebSocket handler uses message.type without proper type definition. Consider defining PR event types in WebSocketMessageType union. 2. Incomplete PR View ActionLocation: Dashboard.tsx:116 The _selectedPRNumber state is declared but never used. Either implement the PR detail view or remove this unused state. 3. Redundant Client-side FilteringLocation: PRList.tsx:330-383 PRs are fetched from API with filter parameter AND filtered locally. This creates inconsistency. Recommend either rely solely on server-side filtering and remove local filter, OR remove status param from API call and do all filtering locally. 4. Missing State Reset in PRMergeDialogLocation: PRMergeDialog.tsx:76-89 Unlike PRCreationDialog, this component doesn't reset mergeMethod, deleteBranch, or error when opening. Add useEffect to reset state on open. 5. PR_STATUS_COLORS Uses Hardcoded ColorsLocation: pullRequest.ts:212-217 Should use Nova design system variables instead: success, primary, destructive, muted pairs. Performance & Security
ConclusionOverall code quality is high. Address the type safety issue with WebSocket events and clean up the redundant filtering and unused state. The implementation is production-ready after these changes. |
|
Review comment posted at #285 (comment) |
- PRList: Use array SWR key with filter for automatic revalidation, remove manual mutate() useEffect since SWR handles filter changes - PRMergeDialog: Add useEffect to reset form state when dialog opens (mergeMethod, deleteBranch, error, isSubmitting) - pullRequest.ts + PRList: Update PR_STATUS_COLORS to use Nova design system variables (bg-success/10, bg-primary/10, bg-destructive/10)
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/__tests__/integration/dashboard-realtime-updates.test.tsx (1)
200-210: Fix pluralization logic in agent count display.The test assertion is correct; it accurately verifies the component's actual output. However, the component at
web-ui/src/components/Dashboard.tsx:675renders{agents.length} agents activewithout pluralization logic, resulting in grammatically incorrect text like "1 agents active" when a single agent is active. Update the component to use proper pluralization: "1 agent active" for singular and "N agents active" for plural.
🧹 Nitpick comments (3)
web-ui/jest.setup.js (1)
38-106: Well-structured global icon mock with factory pattern.The centralized mock factory in
jest.setup.jsis a good approach for consistent icon mocking. However, I notice that the test files (Dashboard.test.tsx,dashboard-realtime-updates.test.tsx,prd-button-sync.test.tsx) each define their own localjest.mock('@hugeicons/react', ...)which will override this global mock.Consider one of these approaches:
- Remove the local mocks from individual test files and rely solely on this global mock
- Keep both if local mocks intentionally use different
data-testidnaming (global uses'Add01Icon', locals use'add-icon')The current setup creates inconsistency: global mock uses icon names directly as
data-testid(e.g.,'Add01Icon'), while local test mocks use normalized names (e.g.,'add-icon'). This could cause confusion when writing assertions.web-ui/__tests__/components/Dashboard.test.tsx (1)
431-474: WebSocket handler tests verify registration but not mutation behavior.The blocker WebSocket tests correctly verify that handlers are registered and called, but the comments at lines 452-453 and 471-473 acknowledge that verifying the actual
mutateBlockerscall is difficult with the mocked SWR. This is acceptable for testing the integration pattern.Consider adding an integration test that actually verifies the blockers list updates after receiving WebSocket events, perhaps using a real SWR provider with controlled responses.
web-ui/__tests__/integration/prd-button-sync.test.tsx (1)
26-80: Icon mock is duplicated across multiple test files.This is the fourth occurrence of the same
createMockIconfactory pattern. While the global mock injest.setup.jsexists, these local mocks override it with differentdata-testidnaming conventions.Consider extracting the mock to a shared test utility file (e.g.,
__tests__/utils/iconMocks.ts) that can be imported and used withjest.mock('@hugeicons/react', () => iconMocks). This would:
- Reduce duplication
- Ensure consistent
data-testidnaming- Make it easier to add new icons in one place
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
web-ui/__tests__/components/Dashboard.test.tsxweb-ui/__tests__/components/PRCreationDialog.test.tsxweb-ui/__tests__/components/PRMergeDialog.test.tsxweb-ui/__tests__/integration/dashboard-realtime-updates.test.tsxweb-ui/__tests__/integration/prd-button-sync.test.tsxweb-ui/jest.setup.js
🚧 Files skipped from review as they are similar to previous changes (1)
- web-ui/tests/components/PRCreationDialog.test.tsx
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: docs/CLAUDE.md:0-0
Timestamp: 2025-11-25T19:08:37.203Z
Learning: Use feature branches from main with Conventional Commits format (feat/fix/docs scope): description
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use Hugeicons (hugeicons/react) for all icons, never mix with lucide-react
Applied to files:
web-ui/jest.setup.jsweb-ui/__tests__/components/Dashboard.test.tsxweb-ui/__tests__/integration/dashboard-realtime-updates.test.tsxweb-ui/__tests__/integration/prd-button-sync.test.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/**/__tests__/**/*.test.{ts,tsx} : Create JavaScript test files colocated or in __tests__/ as *.test.ts
Applied to files:
web-ui/__tests__/components/PRMergeDialog.test.tsxweb-ui/__tests__/components/Dashboard.test.tsxweb-ui/__tests__/integration/dashboard-realtime-updates.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Use React.memo on Dashboard sub-components and useMemo for derived state to optimize performance
Applied to files:
web-ui/__tests__/components/Dashboard.test.tsxweb-ui/__tests__/integration/dashboard-realtime-updates.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Use shadcn/ui components from `@/components/ui/` for UI elements
Applied to files:
web-ui/__tests__/components/Dashboard.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/components/**/*.{ts,tsx} : Use React Context with useReducer for centralized state management in Dashboard
Applied to files:
web-ui/__tests__/components/Dashboard.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/**/*.{ts,tsx} : Include auth token as query parameter in WebSocket connections (?token=TOKEN)
Applied to files:
web-ui/__tests__/components/Dashboard.test.tsx
📚 Learning: 2026-01-11T23:33:31.895Z
Learnt from: CR
Repo: frankbria/codeframe PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-11T23:33:31.895Z
Learning: Applies to web-ui/src/lib/**/*.{ts,tsx} : Implement WebSocket automatic reconnection with exponential backoff (1s → 30s) and full state resync
Applied to files:
web-ui/__tests__/components/Dashboard.test.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/components/**/*.{ts,tsx} : Use functional React components with TypeScript interfaces
Applied to files:
web-ui/__tests__/integration/dashboard-realtime-updates.test.tsx
🧬 Code graph analysis (1)
web-ui/__tests__/components/PRMergeDialog.test.tsx (3)
web-ui/src/types/pullRequest.ts (1)
PullRequest(41-98)web-ui/src/components/pr/PRMergeDialog.tsx (1)
PRMergeDialog(76-219)web-ui/src/lib/api.ts (1)
pullRequestsApi(268-321)
⏰ 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: Frontend Unit Tests
- GitHub Check: Backend Unit Tests
- GitHub Check: E2E Smoke Tests (Chromium)
- GitHub Check: opencode-review
- GitHub Check: Macroscope - Correctness Check
🔇 Additional comments (10)
web-ui/__tests__/components/PRMergeDialog.test.tsx (3)
172-196: Proper use of deferred promise for testing loading state.The loading state test correctly uses a deferred promise pattern to keep the API call pending while asserting the loading UI. The cleanup at line 195 properly resolves the promise to prevent test warnings.
One minor observation: the
resolvePromise!non-null assertion at line 195 is safe here because the promise executor runs synchronously, but you could make this more explicit:♻️ Optional: More explicit typing
- let resolvePromise: (value: unknown) => void; + let resolvePromise!: (value: unknown) => void;Using the definite assignment assertion (
!:) makes the intent clearer that the variable will be assigned before use.
78-81: Good handling of multiple text matches.The test correctly uses
getAllByTextto handle the case where "main" appears multiple times in the dialog (branch info and description). The assertionexpect(mainElements.length).toBeGreaterThan(0)is appropriately flexible.
45-56: Consider resettingonCloseandonSuccessmocks between tests.The
defaultPropsobject is reused across tests, but the mock functions (onClose,onSuccess) are created once. Whilejest.clearAllMocks()inbeforeEachhandles this, be aware that if tests modify the mock implementations, they could leak between tests.web-ui/__tests__/components/Dashboard.test.tsx (2)
14-70: Local mock provides good test isolation with descriptive test IDs.The local mock using descriptive
data-testidvalues (e.g.,'search-icon','git-pull-request-icon') is more readable in test assertions than the raw icon names. This local mock intentionally overrides the global mock injest.setup.js.The categorized comments (PhaseProgress, Dashboard section header, Activity item, UI component, PR component icons) improve maintainability.
1082-1093: Console.error mock cleanup could use safer pattern.The current cleanup in
afterEachchecksjest.isMockFunctionbefore restoring, which is good. However, usingmockRestore()on a spy is the correct approach.web-ui/__tests__/integration/dashboard-realtime-updates.test.tsx (2)
140-151: Good WebSocket message handler tracking pattern.The
messageHandlersMap with unique IDs and cleanup functions is a clean pattern for simulating WebSocket events in tests. The handler tracking allows triggering all registered handlers which properly simulates the real WebSocket behavior.
91-107: Custom SWR mock properly isolates test state.The SWR wrapper correctly disables caching, deduplication, focus/reconnect revalidation, and retry on error. This ensures each test starts with fresh state.
One consideration: the
key: anyandfetcher: anytypes could be more specific, but for test mocks this is acceptable.web-ui/__tests__/integration/prd-button-sync.test.tsx (3)
94-117: SWR mock with mutate tracking for specific endpoints.The SWR mock that tracks
mutatecalls specifically for PRD endpoints (key.includes('/prd')) is a good pattern for verifying cache invalidation behavior without testing SWR internals.
253-281: Good test for project ID filtering on WebSocket messages.The test correctly verifies that PRD completion messages for different projects (project_id: 999 vs 1) are filtered and don't trigger unnecessary cache invalidation. This prevents cross-project state pollution.
326-355: Valuable regression test for tab navigation state persistence.This test addresses the specific bug where the spinner reappears when returning to the Overview tab. The unmount/remount pattern correctly simulates tab navigation. This is a good regression test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Review SummaryThis is a well-implemented PR management feature with excellent test coverage and adherence to project conventions. The implementation follows Nova design system patterns and integrates cleanly with the Dashboard. ✅ Strengths Addressed
|
|
Posted review comment focusing on the key remaining type safety issue with WebSocket event types. The PR is production-ready after adding |
- Add pr_created, pr_merged, pr_closed to WebSocketMessageType union - Update PRList.tsx to use WebSocketMessage type instead of any - Fix Dashboard.tsx pluralization: "1 agent active" vs "N agents active" - Update test assertion to match corrected grammar
| // WebSocket subscription for real-time updates | ||
| useEffect(() => { | ||
| const ws = getWebSocketClient(); | ||
|
|
There was a problem hiding this comment.
Real-time PR updates won’t arrive because ws.subscribe(projectId) isn’t called. Consider subscribing in useEffect before registering the message handler.
| ws.subscribe(projectId); | |
🚀 Want me to fix this? Reply ex: "fix it for me".
Review SummaryThis PR implements a comprehensive PR management UI with excellent architecture and test coverage. All major issues from previous reviews have been addressed. ✅ Fixes Confirmed
💡 Minor Cleanup Suggestions (Optional)
✅ ConclusionThe PR is production-ready. All critical issues (type safety, caching, state management, design system compliance) have been resolved. The remaining items are minor code quality improvements that don't block merge. Test Coverage: 74 tests across 3 components with comprehensive coverage of rendering, validation, actions, and error states. |
|
The review comment has been posted successfully at #285 (comment) All critical issues from previous reviews have been addressed. The PR is production-ready with comprehensive test coverage and proper adherence to project conventions. |
#284, #285) Add comprehensive E2E tests for recently added Git and PR features: Git Visualization (test_git_visualization.spec.ts - 10 tests): - Git section visibility in active/review phases - Branch indicator with dirty state detection - Commit history display - Negative test for discovery phase (hidden) - API integration verification PR Management (test_pr_management.spec.ts - 20 tests): - Tab navigation and panel display - PR list with status filtering - Create PR dialog with form validation - PR card actions (View, Merge buttons) - CI and review status indicators - API integration verification Includes 5 smoke tests for core functionality validation.
This PR implements a complete pull request management interface for the CodeFRAME web UI.
Components
Infrastructure
Dashboard Integration
Tests
Dependencies
Co-Authored-By: Warp [email protected]
Summary by CodeRabbit
New Features
UX / Validation
Realtime & Visibility
Tests
✏️ Tip: You can customize this high-level summary in your review settings.