Skip to content

feat(api): enrich GET /bounties/:id response#86

Open
gavin-openops wants to merge 2 commits intodevasignhq:mainfrom
gavin-openops:fix/bounty-detail-enrichment
Open

feat(api): enrich GET /bounties/:id response#86
gavin-openops wants to merge 2 commits intodevasignhq:mainfrom
gavin-openops:fix/bounty-detail-enrichment

Conversation

@gavin-openops
Copy link

@gavin-openops gavin-openops commented Mar 1, 2026

Summary

  • enrich GET /api/bounties/:id with creator profile data (username + avatar)
  • include assignee profile data when present
  • include application count for the bounty
  • keep 404 behavior for missing bounty IDs

Why

Implements #21 by returning the additional fields needed by the mobile app detail view.

Testing

  • npm test -- bounties_list.test.ts (from packages/api)
  • npm run build (from packages/api)

Closes #21

@devasign-app
Copy link

devasign-app bot commented Mar 1, 2026

🟢 AI Code Review Results

Status: Ready to Merge
Confidence: 100%


🟢 Merge Score: 85/100

🟢 █████████████████░░░ 85%

Recommendation: ✅ This PR looks great and is ready for merge!

The PR successfully enriches the bounty detail endpoint as requested. The implementation is efficient, using Promise.all for concurrent data fetching. However, the code can be improved by using more idiomatic ORM functions and by adding a test case for unassigned bounties to increase robustness.

💡 Code Suggestions (3)

🟡 Medium Priority (1)

  1. packages/api/src/tests/bounties_list.test.ts (Line 228)
    ✨ The test suite is missing coverage for a key scenario: fetching a bounty that has no assignee.

💭 Reasoning: The current tests only cover bounties with an assignee. Adding a test for an unassigned bounty ensures the conditional logic for fetching the assignee is correct and prevents future regressions where assignee: null might be handled incorrectly.

Suggested Code:

    it('should return null for assignee when bounty is not assigned', async () => {
        const bountyId = 'bounty-no-assignee';
        vi.mocked(db.query.bounties.findFirst).mockResolvedValue({
            id: bountyId,
            title: 'Bounty 2',
            status: 'open',
            creatorId: 'creator-1',
            assigneeId: null, // No assignee
        } as any);

        vi.mocked(db.query.users.findFirst)
            .mockResolvedValueOnce({ username: 'creatorUser', avatarUrl: 'https://avatar/creator.png' } as any);

        vi.mocked(db.select as any).mockReturnValue({
            from: () => ({
                where: async () => [{ count: 1 }],
            }),
        } as any);

        const res = await app.request(`/api/bounties/${bountyId}`, {
            headers: {
                'Authorization': 'Bearer valid.token'
            }
        });

        expect(res.status).toBe(200);
        const body = await res.json();

        expect(body.id).toBe(bountyId);
        expect(body.creator).not.toBeNull();
        expect(body.assignee).toBeNull();
        expect(body.applicationCount).toBe(1);
    });

🔵 Low Priority (2)

  1. packages/api/src/routes/bounties.ts (Line 153)
    🎨 Use Drizzle ORM's count() aggregate function instead of a raw sql fragment for consistency and type safety. You'll need to import count from drizzle-orm.

💭 Reasoning: Using the ORM's built-in functions like count() is more idiomatic, maintainable, and less prone to errors than raw SQL strings. It ensures the data access layer remains consistent across the application.

Suggested Code:

        db
            .select({ count: count() })
            .from(applications)
            .where(eq(applications.bountyId, id)),
  1. packages/api/src/tests/bounties_list.test.ts
    🎨 The test file bounties_list.test.ts now contains tests for both the list and detail endpoints. Consider renaming it to bounties.test.ts to better reflect its contents.

💭 Reasoning: A more accurate filename improves project organization and makes it easier for developers to locate relevant tests, enhancing overall maintainability.

📊 Review Metadata
  • Processing Time: 78s
  • Analysis Date: 3/1/2026, 8:55:30 PM

🤖 This review was generated by AI. While we strive for accuracy, please use your judgment when applying suggestions.

💬 Questions about this review? Open an issue or contact support.

@devasign-app
Copy link

devasign-app bot commented Mar 1, 2026

🔄 Follow-Up AI Code Review

Status: Ready to Merge
Confidence: 100%


🟢 Updated Merge Score: 100/100

🟢 ████████████████████ 100%

Recommendation: ✅ This PR looks great and is ready for merge!

📋 Previous Review Summary

The PR successfully enriches the bounty detail endpoint as requested. The implementation is efficient, using Promise.all for concurrent data fetching. However, the code can be improved by using more idiomatic ORM functions and by adding a test case for unassigned bounties to increase robustness.

Excellent work. The feedback from the previous review has been fully addressed. The addition of the test case for bounties without an assignee makes the implementation robust. The code is clean, efficient, and ready for merging.

💡 Code Suggestions

✨ Great job! No specific suggestions at this time.

📊 Review Metadata
  • Processing Time: 58s
  • Analysis Date: 3/1/2026, 9:25:31 PM
  • Review Type: Follow-Up (triggered by new push)

🤖 This is a follow-up review generated after new commits were pushed to the PR.

💬 Questions? Open an issue or contact support.

@gavin-openops
Copy link
Author

gavin-openops commented Mar 1, 2026

I implemented the medium-priority suggestion and pushed an update:

  • Added test coverage for the unassigned bounty case (assignee: null) in bounties_list.test.ts

I left the count() ORM refactor as-is for now to keep this PR scoped to bounty acceptance, but happy to do that as a follow-up if you want it included here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement GET /bounties/:id — bounty detail

1 participant