feat(dashboard): add PRs page with project filter and run counts#688
feat(dashboard): add PRs page with project filter and run counts#688
Conversation
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean implementation that follows existing patterns well (mirrors workitems page closely). Two issues worth addressing — one correctness bug with React keys and one missing test coverage for the new code path.
Code Issues
Should Fix
-
web/src/components/prs/prs-table.tsx:46—key={item.prNumber}will produce duplicate React keys in the org-wide ("All projects") view. TheprNumberis unique per project (viaUNIQUE(projectId, prNumber)constraint), but whenlistPRsForOrgaggregates across projects, two different repos can both have PR #1. The GROUP BY correctly produces separate rows (due to differentrepoFullName), but React will see duplicate keys and may exhibit incorrect rendering/update behavior. Consider using a composite key likekey={`${item.repoFullName}-${item.prNumber}`}. -
web/src/routes/prs.tsx/web/src/components/prs/prs-table.tsx— In the org-wide view, PRs from different repositories are indistinguishable. TherepoFullNameis returned by the backend but never displayed. When a user selects "All projects", two PRs with the same number from different repos would appear identical. Consider showingrepoFullNameas a secondary label, at least when no project filter is active. -
tests/unit/api/routers/prs.test.ts— The unit test mock setup (line 14-17) does not includelistPRsForOrg, and there are no tests for thelist({})/list({ projectId: undefined })code path (org-wide query). The new branch in the router whereprojectIdis omitted is completely untested. The mock at line 14 will cause the import to fail orlistPRsForOrgto be undefined when invoked.
| </tr> | ||
| )} | ||
| {pageItems.map((item) => ( | ||
| <tr |
There was a problem hiding this comment.
[SHOULD_FIX] key={item.prNumber} will produce duplicate React keys in the org-wide view. prNumber is only unique within a project (UNIQUE(projectId, prNumber)), so two repos can both have PR #1. This causes incorrect React reconciliation.
Fix: use a composite key:
key={`${item.repoFullName}-${item.prNumber}`}Also consider: repoFullName is not shown in the table but is available from the backend — when viewing "All projects", users can't distinguish which repo a PR belongs to.
There was a problem hiding this comment.
Fixed! Changed the React key to use a composite key ${item.repoFullName}-${item.prNumber} to ensure uniqueness across different repositories in the org-wide view.
Also added a showRepoName prop to the table that displays the repository name in a separate column when viewing "All projects" (when no project filter is active). This makes it clear which repo each PR belongs to.
| await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); | ||
| return listPRsForProject(input.projectId); | ||
| } | ||
| return listPRsForOrg(ctx.effectiveOrgId); |
There was a problem hiding this comment.
[SHOULD_FIX] The listPRsForOrg path is not covered by unit tests. The mock setup in prs.test.ts doesn't mock listPRsForOrg, so calling list({}) (no projectId) would hit an undefined function. Add a test case for the org-wide path and mock listPRsForOrg.
There was a problem hiding this comment.
Fixed! Added listPRsForOrg to the mock setup and created a new test case "returns PRs across all projects when no projectId given" that tests the org-wide query path. The test verifies that when list({}) is called without a projectId, it calls listPRsForOrg with the org ID and skips the project access verification.
- Fix duplicate React keys by using composite key (repoFullName + prNumber) - Add repoFullName column when viewing all projects to distinguish PRs from different repos - Add missing listPRsForOrg mock and test coverage for org-wide query path Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Review Feedback AddressedAll review feedback has been addressed and changes have been pushed: Changes Made
Verification✅ All tests passing (12/12 tests in prs.test.ts) |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured PR that follows established patterns. The new PRs page mirrors the existing Work Items page across all layers (route, table component, tRPC endpoint, sidebar nav, repository). All CI checks pass. Approving with two minor suggestions.
Should Fix
-
tests/unit/api/routers/prs.test.ts:43-51 —
mockPRSummaryis missing therunCountfield that was added to thePRSummaryinterface. The mock should includerunCount: 3(or similar) to accurately reflect the real data shape. Won't cause test failures since the test compares against its own mock, but makes the test less realistic and could mask type issues if someone copies this mock elsewhere. -
Integration test gap —
listPRsForOrghas no integration test coverage (the integration test file wasn't modified). Since the function follows the exact same pattern as the already-testedlistWorkItems, this is low risk, but worth adding totests/integration/db/prWorkItemsRepository.test.tsin a follow-up.
|
Checking the pipeline and moving the next card for the PRs page |
2 participants
Summary
/prsroute page listing all PRs with PR number (clickable GitHub link), title, linked work item (clickable link), and run countprs-table.tsxcomponent following theworkitems-table.tsxpattern with pagination and empty stateGitPullRequestlucide iconprs.listtRPC endpoint to support optionalprojectId(shows all org PRs when omitted)listPRsForProjectandlistPRsForWorkItemrepository functions to includerunCountvia JOIN withagentRunslistPRsForOrgrepository function for cross-project PR listingTest plan
/prs— page loads and shows all PRsCloses https://trello.com/c/69adf50de48c9db558e6aa7d
🤖 Generated with Claude Code