feat(dashboard): add Work Items page with project filter and pagination#687
feat(dashboard): add Work Items page with project filter and pagination#687
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean implementation that follows existing patterns well. No blocking issues — the code is correct, secure, and follows the established conventions (e.g., the webhook logs page pattern). A couple of observations below.
Should Fix
-
src/db/repositories/prWorkItemsRepository.ts:71-99 —
listWorkItemsForProjectis now dead code from production's perspective. The router was its only caller and now useslistWorkItemsinstead. The old function is still exported and tested in integration tests, but it's functionally superseded bylistWorkItems(orgId, projectId). Consider removing it (and updating integration tests to uselistWorkItems) to avoid confusion about which function to call. -
Client-side pagination vs server-side — The webhook logs page uses server-side pagination (sends
limit/offsetto the API), while this page fetches all work items and paginates client-side. This is fine for now but worth noting: if an org accumulates thousands of work items, the full fetch will get expensive. The PR description explicitly calls this out as client-side pagination, so this is a conscious choice — just flagging for future consideration.
- Remove listWorkItemsForProject function that was superseded by listWorkItems - Update integration tests to use listWorkItems instead - All tests pass (4261 tests) Addresses review feedback on PR #687 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Review Feedback AddressedThank you for the review! I've addressed the feedback: Changes Made✅ Removed dead code: Deleted
Client-side pagination noteThe client-side pagination approach is noted. This works well for the current scale, and we can migrate to server-side pagination if/when work item counts grow large enough to impact performance. Verification
|
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean implementation that follows existing codebase patterns well. One correctness bug in the prCount aggregation for the new org-wide (no projectId) query path.
Code Issues
Should Fix
-
src/db/repositories/prWorkItemsRepository.ts:95 —
countDistinct(prWorkItems.prNumber)undercounts PRs in the org-wide query. TheprNumbercolumn is only unique within a project (per theuq_pr_work_items_project_prconstraint). WhenprojectIdis omitted and the query spans multiple projects, two different PRs (e.g., project-A/PR#1 and project-B/PR#1) linked to the same work item will be counted as one. This was safe before this PR because the old query always filtered to a singleprojectId. UsecountDistinct(prWorkItems.id)instead, which is globally unique. -
Integration test gap: The integration tests for
listWorkItemswere updated to passorgIdbut none test the org-wide path (calling withoutprojectId). The cross-project aggregation bug above would be caught by a test that links the same work item to PRs in two different projects, then callslistWorkItems(orgId)without aprojectIdand asserts the combinedprCount.
| ), | ||
| ) | ||
| .where(and(eq(prWorkItems.projectId, projectId), isNotNull(prWorkItems.workItemId))) | ||
| .where(and(...conditions)) |
There was a problem hiding this comment.
The .where(and(...conditions)) here groups across multiple projects in the org-wide path, but the prCount above uses countDistinct(prWorkItems.prNumber). Since prNumber is only unique within a project, two distinct PRs from different projects with the same number (e.g., project-A/#1 and project-B/#1) linked to the same work item would be counted as 1 instead of 2.
This was safe before this PR because the old query always filtered to a single projectId.
Fix: use countDistinct(prWorkItems.id) instead — the id column is a globally unique UUID, so it correctly counts distinct PR rows regardless of cross-project grouping.
There was a problem hiding this comment.
Fixed! Changed to countDistinct(prWorkItems.id) to correctly count PRs across multiple projects. Also added an integration test that links the same work item to PRs with duplicate numbers across different projects to catch this bug.
- Changed prCount aggregation from countDistinct(prNumber) to countDistinct(id) - prNumber is only unique within a project, not globally - Added integration test for org-wide query with same PR numbers across projects Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — well-structured feature addition that follows existing patterns in the codebase.
The previous reviewer's countDistinct(prWorkItems.prNumber) → countDistinct(prWorkItems.id) fix was correctly applied and verified with a targeted integration test. The authorization model is sound: org-scoped access is enforced via the project-IDs subquery when no projectId is specified, and verifyProjectOrgAccess gates the single-project path.
Minor observation (not blocking): This page uses client-side pagination (fetch all, slice in browser) while the analogous Webhook Logs page uses server-side pagination. This is fine at current scale but worth noting if work item counts grow large — it would be a straightforward follow-up to add limit/offset to the tRPC query.
|
Checking the pipeline and moving the Work Items page PR to TODO |
Summary
/workitemsroute listing all tracked work items with PR count and run countClipboardListicon)Technical Details
New files:
web/src/routes/workitems.tsx— Work Items page with project filter, usingtrpc.workItems.listqueryweb/src/components/workitems/workitems-table.tsx— Table component with work item title (clickable external link), PR count, run count, and client-side paginationUpdated files:
src/api/routers/workItems.ts— ChangedprojectIdfrom required to optional; when omitted, returns work items across all projects in the orgsrc/db/repositories/prWorkItemsRepository.ts— AddedlistWorkItems(orgId, projectId?)function for org-wide or project-filtered queriesweb/src/components/layout/sidebar.tsx— Added Work Items link withClipboardListicon between Runs and Webhook Logsweb/src/routes/route-tree.ts— RegisteredworkItemsRoutetests/unit/api/routers/workItems.test.ts— Updated tests for newlistWorkItemsAPI and added test for org-wide listingTest plan
/workitemsTrello card: https://trello.com/c/69adf4ec5c05ef22cb2e62a2
🤖 Generated with Claude Code