Skip to content

feat(api): enhance GET /api/bounties/:id detail response#87

Open
Jujubee-LLM wants to merge 2 commits intodevasignhq:mainfrom
Jujubee-LLM:feat/bounty-detail-endpoint-21
Open

feat(api): enhance GET /api/bounties/:id detail response#87
Jujubee-LLM wants to merge 2 commits intodevasignhq:mainfrom
Jujubee-LLM:feat/bounty-detail-endpoint-21

Conversation

@Jujubee-LLM
Copy link

@Jujubee-LLM Jujubee-LLM commented Mar 2, 2026

/claim #21
Closes #21

Added fields

  • creator: { id, username, avatar }
  • applicationCount
  • assignee: { id, username, avatar } | null
  • keeps existing bounty fields (including status)

Files changed

  • packages/api/src/routes/bounties.ts
  • packages/api/src/__tests__/bounties_detail.test.ts

Validation

  • pnpm -r test ✅ (104 passed)
  • pnpm --filter @devasign/api build

@devasign-app
Copy link

devasign-app bot commented Mar 2, 2026

🟠 AI Code Review Results

Status: Changes Needed
Confidence: 100%


🟠 Merge Score: 65/100

🔴 █████████████░░░░░░░ 65%

Recommendation: ❌ This PR needs significant improvements before it should be merged.

The PR successfully adds the required fields to the bounty detail endpoint. However, it introduces a significant performance issue by making multiple sequential database queries instead of a single, more efficient one. There are also minor issues with error handling and test brittleness that should be addressed.

💡 Code Suggestions (3)

🔴 High Priority (1)

  1. packages/api/src/routes/bounties.ts (Line 136)
    ⚡ The current implementation executes up to 4 separate database queries for a single API request (bounty, application count, creator, assignee), which is inefficient and will lead to performance degradation.

💭 Reasoning: Consolidating data retrieval into fewer queries significantly reduces database round trips, improving API response time and scalability. Drizzle ORM's relational queries are designed for this purpose, resulting in cleaner and more performant code.

Suggested Code:

bountiesRouter.get('/:id', async (c) => {
    const id = c.req.param('id');

    // This assumes relations are defined in your schema, e.g.:
    // export const bountiesRelations = relations(bounties, ({ one }) => ({
    //   creator: one(users, { fields: [bounties.creatorId], references: [users.id] }),
    //   assignee: one(users, { fields: [bounties.assigneeId], references: [users.id] }),
    // }));
    const bounty = await db.query.bounties.findFirst({
        where: eq(bounties.id, id),
        with: {
            creator: {
                columns: {
                    id: true,
                    username: true,
                    avatarUrl: true,
                },
            },
            assignee: {
                columns: {
                    id: true,
                    username: true,
                    avatarUrl: true,
                },
            },
        },
    });

    if (!bounty) {
        return c.json({ error: 'Bounty not found' }, 404);
    }

    if (!bounty.creator) {
        console.error(`Data integrity issue: Creator with ID ${bounty.creatorId} not found for bounty ${id}`);
        return c.json({ error: 'Internal Server Error' }, 500);
    }

    const [applicationCountResult] = await db
        .select({ count: sql<number>`cast(count(*) as int)` })
        .from(applications)
        .where(eq(applications.bountyId, id));

    return c.json({
        ...bounty,
        creator: {
            id: bounty.creator.id,
            username: bounty.creator.username,
            avatar: bounty.creator.avatarUrl,
        },
        applicationCount: applicationCountResult?.count ?? 0,
        assignee: bounty.assignee
            ? {
                id: bounty.assignee.id,
                username: bounty.assignee.username,
                avatar: bounty.assignee.avatarUrl,
            }
            : null,
    });
});

🟡 Medium Priority (1)

  1. packages/api/src/routes/bounties.ts (Line 93)
    🔧 The catch block at line 93 is empty (catch {}). This practice, known as "swallowing errors," hides potential bugs during cursor parsing and makes debugging extremely difficult.

💭 Reasoning: Proper error handling should log the error for observability and return a clear error message to the client. This ensures that issues are surfaced immediately rather than failing silently.

Suggested Code:

        } catch (err) {
            console.error("Invalid cursor provided:", err);
            return c.json({ error: 'Invalid cursor' }, 400);
        }

🔵 Low Priority (1)

  1. packages/api/src/tests/bounties_detail.test.ts (Line 70)
    ✨ The tests are tightly coupled to the implementation's use of multiple db.select calls, making them brittle. The use of chained mockReturnValueOnce relies on the exact order of database calls.

💭 Reasoning: After refactoring the route handler to use relational queries, the tests can be simplified. Mocking the higher-level db.query.bounties.findFirst API makes tests more robust and less sensitive to the specific implementation details of the data access layer.

Suggested Code:

    it('should return full bounty detail including creator, application count, and assignee', async () => {
        vi.mocked(db.query.bounties.findFirst).mockResolvedValue({
            id: 'bounty-1',
            title: 'Test bounty',
            description: 'Test description',
            creatorId: 'creator-1',
            assigneeId: 'assignee-1',
            status: 'assigned',
            creator: {
                id: 'creator-1',
                username: 'alice',
                avatarUrl: 'https://img/creator.png',
            },
            assignee: {
                id: 'assignee-1',
                username: 'bob',
                avatarUrl: 'https://img/assignee.png',
            },
        } as any);

        vi.mocked(db.select).mockResolvedValue([{ count: 3 }]);

        const res = await app.request('/api/bounties/bounty-1', {
            headers: {
                Authorization: 'Bearer valid.token',
            },
        });

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

        expect(body.applicationCount).toBe(3);
        expect(body.creator.id).toBe('creator-1');
        expect(body.assignee.id).toBe('assignee-1');
    });
📊 Review Metadata
  • Processing Time: 95s
  • Analysis Date: 3/2/2026, 3:41:57 AM

🤖 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 2, 2026

🔄 Follow-Up AI Code Review

Status: Ready to Merge
Confidence: 100%


🟢 Updated Merge Score: 95/100

🟢 ███████████████████░ 95%

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

📋 Previous Review Summary

The PR successfully adds the required fields to the bounty detail endpoint. However, it introduces a significant performance issue by making multiple sequential database queries instead of a single, more efficient one. There are also minor issues with error handling and test brittleness that should be addressed.

Excellent follow-up. The author has successfully addressed all major concerns from the previous review. The critical performance issue caused by sequential database queries has been resolved by implementing a parallelized query approach using Promise.all. Furthermore, robust error handling for missing data (e.g., a bounty's creator) has been added, and the test suite has been significantly improved to cover these important edge cases. The code quality is now very high, and the PR is in great shape for merging. The one minor suggestion is a non-blocking style improvement for better long-term maintainability.

💡 Code Suggestions (1)

🔵 Low Priority (1)

  1. packages/api/src/routes/bounties.ts (Line 11)
    ✨ The toPublicUser function has an explicitly defined inline type for its user parameter. While this works, a more maintainable and robust approach would be to derive this type from the Drizzle schema itself. This ensures the function's signature automatically stays in sync with the database model.

💭 Reasoning: Using types derived from the schema (e.g., using typeof users.$inferSelect) reduces manual type definitions and prevents potential drift between application code and the database schema. It makes the code more resilient to future changes in the users table.

Suggested Code:

import { bounties, applications, users } from '../db/schema';
import { eq, and, gte, lte, sql, desc, or, lt, inArray } from 'drizzle-orm';

const bountiesRouter = new Hono<{ Variables: Variables }>();

// Define a type for the public user data based on the schema
type PublicUserData = Pick<typeof users.$inferSelect, 'id' | 'username' | 'avatarUrl'>;

const toPublicUser = (user: PublicUserData) => ({
    id: user.id,
    username: user.username,
    avatarUrl: user.avatarUrl,
});
📊 Review Metadata
  • Processing Time: 241s
  • Analysis Date: 3/2/2026, 3:05:44 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.

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