feat(api): enrich GET /bounties/:id with creator, assignee, and application count#91
Conversation
…on count and tests
🟠 AI Code Review ResultsStatus: Changes Needed 🟠 Merge Score: 65/100🔴 Recommendation: ❌ This PR needs significant improvements before it should be merged. The PR successfully enriches the bounty detail endpoint and adds good test coverage. However, there is a critical performance issue in how the application count is calculated that must be addressed before merging. A minor code redundancy was also found. 💡 Code Suggestions (1)🔴 High Priority (1)
💭 Reasoning: Loading all related application records into memory just to get a count scales poorly and will cause performance degradation on bounties with many applicants. Using a database-level aggregation (e.g., a subquery with Suggested Code: bountiesRouter.get('/:id', async (c) => {
const id = c.req.param('id');
const bounty = await db.query.bounties.findFirst({
where: eq(bounties.id, id),
columns: {
id: true,
githubIssueId: true,
repoOwner: true,
repoName: true,
title: true,
description: true,
amountUsdc: true,
techTags: true,
difficulty: true,
status: true,
deadline: true,
creatorId: true,
assigneeId: true,
createdAt: true,
updatedAt: true,
},
with: {
creator: {
columns: {
id: true,
username: true,
avatarUrl: true,
},
},
assignee: {
columns: {
id: true,
username: true,
avatarUrl: true,
},
},
},
extras: {
application_count: sql<number>`(SELECT count(*) FROM ${applications} WHERE ${applications.bountyId} = ${bounties.id})`.as('application_count'),
},
});
if (!bounty) {
return c.json({ error: 'Bounty not found' }, 404);
}
const { creator, assignee, ...base } = bounty;
return c.json({
...base,
creator: creator
? {
id: creator.id,
username: creator.username,
avatar: creator.avatarUrl,
}
: null,
assignee: assignee
? {
id: assignee.id,
username: assignee.username,
avatar: assignee.avatarUrl,
}
: null,
});
});📊 Review Metadata
|
❌ PR Review FailedError DetailsThe PR review system encountered an error while analyzing this pull request: What to do next
|
|
Updated tests per review:
Validation run: |
🔄 Follow-Up AI Code ReviewStatus: Ready to Merge 🟢 Updated Merge Score: 95/100🟢 Recommendation: ✅ This PR looks great and is ready for merge! 📋 Previous Review SummaryThe PR successfully enriches the bounty detail endpoint and adds good test coverage. However, there is a critical performance issue in how the application count is calculated that must be addressed before merging. A minor code redundancy was also found. Excellent work addressing the previous feedback. The critical performance issue with the application count has been resolved effectively by using a direct database count query. The code has also been refactored to be much cleaner and more maintainable. The PR is in great shape. I've added one minor, non-blocking suggestion to parallelize the database queries for a small performance gain. 💡 Code Suggestions (1)🔵 Low Priority (1)
💭 Reasoning: Running independent database queries in parallel reduces the total wait time to the duration of the longest query, rather than the sum of both queries' durations. This makes the endpoint more resilient to database load and slightly faster. Suggested Code: const [bounty, [{ count: applicationCount }]] = await Promise.all([
db.query.bounties.findFirst({
where: eq(bounties.id, id),
with: {
creator: {
columns: {
username: true,
avatarUrl: true,
},
},
assignee: {
columns: {
id: true,
username: true,
avatarUrl: true,
},
},
},
}),
db.select({ count: count() }).from(applications).where(eq(applications.bountyId, id)),
]);
if (!bounty) {
return c.json({ error: 'Bounty not found' }, 404);
}
return c.json({
...bounty,
applicationCount,
});📊 Review Metadata
|
1 participant
Fixes #21
Changes
/claim #21