fix: cap talent search limit at 50 in service layer#1439
Conversation
github-actions
Bot
added
bug
|
Hi @vidhiii1711, thanks for contributing to InternHack! 🎉 I have automatically:
Our workflows will now analyze your changes to classify:
Tip Ensure your PR description references the issue it resolves (e.g. Happy coding! 🚀 |
github-actions
Bot
added
scope:backend
📝 WalkthroughWalkthroughAdds a Prisma import to the opensource routes so the ChangesBackend Database and Query Limits
Sequence Diagram(s)sequenceDiagram
participant Client
participant RecruiterService as searchTalent
participant Prisma as prisma.user.findMany
Client->>RecruiterService: request with filter.limit
RecruiterService->>RecruiterService: safeLimit = min(filter.limit, 50)
RecruiterService->>Prisma: findMany(skip, take = safeLimit)
Prisma-->>RecruiterService: records
RecruiterService-->>Client: results + pagination(limit: safeLimit, totalPages: computedFromFilter)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/module/opensource/opensource.routes.ts (1)
30-62: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove database queries to service layer per coding guidelines.
The inline handler violates the architectural boundaries defined in the coding guidelines:
- "Routes file should contain Express router with middleware chain"
- "Service layer must contain business logic, database queries, and external API calls"
Lines 32-35 duplicate the existing
getMyRepoRequests(userId)method already present inopensource.service.ts(see context snippet 3). Additionally, all other routes in this file delegate to controller methods (e.g., lines 25-27, 72-73), but this handler contains inline business logic and direct Prisma calls.Refactor this handler to delegate to a controller method that orchestrates service layer calls for the database queries and enrichment logic.
♻️ Suggested refactor approach
Move the enrichment logic into a new service method (or enhance the existing
getMyRepoRequests), then delegate from the route to a controller method:In
opensource.service.ts:async getMyRepoRequestsWithRepoId(userId: number) { const requests = await prisma.repoRequest.findMany({ where: { userId }, orderBy: { createdAt: "desc" }, }); const approvedUrls = requests .filter((r) => r.status === "APPROVED") .map((r) => r.url); const approvedRepos = approvedUrls.length > 0 ? await prisma.opensourceRepo.findMany({ where: { url: { in: approvedUrls } }, select: { id: true, url: true }, }) : []; const repoIdByUrl = new Map(approvedRepos.map((r) => [r.url, r.id])); return requests.map((r) => ({ ...r, repoId: r.status === "APPROVED" ? (repoIdByUrl.get(r.url) ?? null) : null, })); }In
opensource.controller.ts:async getMyRepoRequests(req: Request, res: Response, next: NextFunction) { try { const requests = await this.service.getMyRepoRequestsWithRepoId(req.user!.id); res.json({ requests }); } catch (err) { next(err); } }In this routes file:
-opensourceRouter.get("/requests/mine", authMiddleware, requireRole("STUDENT"), async (req, res, next) => { - try { - const requests = await prisma.repoRequest.findMany({ - where: { userId: req.user!.id }, - orderBy: { createdAt: "desc" }, - }); - - // For approved requests, look up the corresponding InternHack repo ID so - // the client can open the repo detail popup directly. - const approvedUrls = requests - .filter((r) => r.status === "APPROVED") - .map((r) => r.url); - - const approvedRepos = - approvedUrls.length > 0 - ? await prisma.opensourceRepo.findMany({ - where: { url: { in: approvedUrls } }, - select: { id: true, url: true }, - }) - : []; - - const repoIdByUrl = new Map(approvedRepos.map((r) => [r.url, r.id])); - - const enriched = requests.map((r) => ({ - ...r, - repoId: r.status === "APPROVED" ? (repoIdByUrl.get(r.url) ?? null) : null, - })); - - res.json({ requests: enriched }); - } catch (err) { - next(err); - } -}); +opensourceRouter.get("/requests/mine", authMiddleware, requireRole("STUDENT"), (req, res, next) => + controller.getMyRepoRequests(req, res, next) +);Once refactored, the
prismaimport on line 1 can be removed from this routes file.As per coding guidelines: "Routes file should contain Express router with middleware chain" and "Service layer must contain business logic, database queries, and external API calls".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/module/opensource/opensource.routes.ts` around lines 30 - 62, The route handler for GET "/requests/mine" in opensource.routes.ts contains business logic and direct Prisma queries; move that logic into the service layer by adding or updating a method like getMyRepoRequestsWithRepoId(userId) in opensource.service.ts that performs the repoRequest.findMany, approvedUrls filtering, opensourceRepo.findMany lookup, map creation, and returns the enriched requests; then add a controller method getMyRepoRequests in opensource.controller.ts that calls service.getMyRepoRequestsWithRepoId(req.user!.id) and sends res.json({ requests }), and finally change the route to delegate to that controller method (removing prisma usage and the inline logic from opensource.routes.ts).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/module/recruiter/recruiter.service.ts`:
- Around line 656-663: The pagination computes DB reads with safeLimit but still
uses raw filter.limit for metadata; update the metadata construction (where
totalPages/pageSize/current page info is built around the results from
prisma.user.findMany and total) to use safeLimit instead of filter.limit (and
ensure any calculations that derive skip or totalPages use safeLimit as the page
size), referencing the existing variables safeLimit, skip, filter.page,
students, and total so page metadata matches the actual queried page size.
---
Outside diff comments:
In `@server/src/module/opensource/opensource.routes.ts`:
- Around line 30-62: The route handler for GET "/requests/mine" in
opensource.routes.ts contains business logic and direct Prisma queries; move
that logic into the service layer by adding or updating a method like
getMyRepoRequestsWithRepoId(userId) in opensource.service.ts that performs the
repoRequest.findMany, approvedUrls filtering, opensourceRepo.findMany lookup,
map creation, and returns the enriched requests; then add a controller method
getMyRepoRequests in opensource.controller.ts that calls
service.getMyRepoRequestsWithRepoId(req.user!.id) and sends res.json({ requests
}), and finally change the route to delegate to that controller method (removing
prisma usage and the inline logic from opensource.routes.ts).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47c68dec-841e-4858-ae07-5db6b2bc6d9d
📒 Files selected for processing (2)
server/src/module/opensource/opensource.routes.tsserver/src/module/recruiter/recruiter.service.ts
vidhiii1711
force-pushed
the
fix/talent-search-limit-cap
branch
from
e727e1d to
60f10eb
Compare
github-actions
Bot
added
level:critical
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/src/module/recruiter/recruiter.service.ts (1)
737-737:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
safeLimitfortotalPagescalculation.Line 737 still calculates
totalPagesusingfilter.limit, but the actual page size issafeLimit(line 664). This creates a pagination inconsistency: when a client requestslimit=100(capped to 50),totalPagesis computed asMath.ceil(total / 100)instead ofMath.ceil(total / 50), resulting in fewer reported pages than actually needed to retrieve all records.For example, with 150 total records and
limit=100, the response would showtotalPages: 2(incorrect) instead oftotalPages: 3(correct). This breaks client-side pagination logic.🔧 Proposed fix
pagination: { page: filter.page, limit: safeLimit, total, - totalPages: Math.ceil(total / filter.limit), + totalPages: Math.ceil(total / safeLimit), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/module/recruiter/recruiter.service.ts` at line 737, The totalPages calculation is using filter.limit but should use the actual page size safeLimit (declared earlier), so update the assignment for totalPages to use Math.ceil(total / safeLimit) (and guard against zero by using Math.ceil(total / (safeLimit || 1))) to ensure pagination reflects the applied cap; change the expression that sets totalPages (currently referencing filter.limit) to reference safeLimit instead.
🧹 Nitpick comments (1)
server/src/module/recruiter/recruiter.service.ts (1)
355-363: ⚖️ Poor tradeoffConsider moving the duplicate status check inside the transaction.
The early-return check at Line 356 prevents unnecessary database writes and emails when the status is unchanged, which is good. However, because this check occurs outside the transaction (Line 361), there's a narrow race window where the status could theoretically change between the check and the transaction execution.
The
_updateApplicationStatushelper at Line 821 does include its own status comparison before creating history, which mitigates the risk. Still, for consistency and to fully prevent race conditions, consider moving the duplicate check inside the transaction.♻️ Optional refactor to move check inside transaction
- // Fix for `#1111`: Prevent duplicate status updates and emails - if (application.status === status) { - const { job: _job, student: _student, ...current } = application; - return current; - } - const updated = await prisma.$transaction(async (tx) => { + const current = await tx.application.findUnique({ + where: { id: applicationId }, + select: { status: true }, + }); + if (current?.status === status) { + return await tx.application.findUnique({ where: { id: applicationId } }); + } return this._updateApplicationStatus(tx, applicationId, status, recruiterId); });Note: You would also need to move the email logic inside or after the transaction to respect the short-circuit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/module/recruiter/recruiter.service.ts` around lines 355 - 363, The duplicate-status early return (comparing application.status to status) should be performed inside the database transaction to avoid the race window between the check and prisma.$transaction; move the comparison into the transaction block that calls this._updateApplicationStatus (or delegate to that helper) and ensure any email-sending logic that relies on the short-circuit is moved to after the transaction completes (or executed within the same tx-controlled flow) so no emails are sent when the status stayed the same; reference applicationId, recruiterId, prisma.$transaction and the _updateApplicationStatus helper when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/src/module/recruiter/recruiter.service.ts`:
- Around line 477-485: The current catch in recruiter.service.ts using
JSON.parse(JSON.stringify(evaluationScores)) doesn't validate client input;
replace this with a proper schema validation at the request boundary (or inside
the service) for evaluationScores: define a Zod (or Joi) schema that describes
the expected record shape (keys -> { score: number, comment?: string }),
validate evaluationScores (e.g., safeParse) and on failure throw or return the
existing 422 error with validation details; remove the
JSON.parse(JSON.stringify(...)) deep-clone and use the validated value as
parsedScores so downstream code uses a type-safe, validated object.
---
Duplicate comments:
In `@server/src/module/recruiter/recruiter.service.ts`:
- Line 737: The totalPages calculation is using filter.limit but should use the
actual page size safeLimit (declared earlier), so update the assignment for
totalPages to use Math.ceil(total / safeLimit) (and guard against zero by using
Math.ceil(total / (safeLimit || 1))) to ensure pagination reflects the applied
cap; change the expression that sets totalPages (currently referencing
filter.limit) to reference safeLimit instead.
---
Nitpick comments:
In `@server/src/module/recruiter/recruiter.service.ts`:
- Around line 355-363: The duplicate-status early return (comparing
application.status to status) should be performed inside the database
transaction to avoid the race window between the check and prisma.$transaction;
move the comparison into the transaction block that calls
this._updateApplicationStatus (or delegate to that helper) and ensure any
email-sending logic that relies on the short-circuit is moved to after the
transaction completes (or executed within the same tx-controlled flow) so no
emails are sent when the status stayed the same; reference applicationId,
recruiterId, prisma.$transaction and the _updateApplicationStatus helper when
making this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 71e87d59-01d8-4ebb-994e-2c6389e943b3
📒 Files selected for processing (2)
server/src/module/opensource/opensource.routes.tsserver/src/module/recruiter/recruiter.service.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/module/opensource/opensource.routes.ts
| // Fix for #1116: Gracefully catch malformed JSON data during evaluation | ||
| let parsedScores; | ||
| try { | ||
| parsedScores = JSON.parse(JSON.stringify(evaluationScores)); | ||
| } catch (error) { | ||
| const err = new Error("Invalid JSON format in evaluation data"); | ||
| (err as any).status = 422; | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
JSON parsing doesn't validate malformed input as intended.
The comment states this is "Fix for #1116: Gracefully catch malformed JSON data during evaluation," but JSON.parse(JSON.stringify(evaluationScores)) only performs deep cloning. If evaluationScores is already a JavaScript object (which it is after Express middleware parses the request body), this operation won't fail unless there are circular references—not the same as catching malformed JSON from the client.
To properly validate the structure and types of evaluationScores, use schema validation (e.g., Zod) in the controller or route validation layer before calling this service method.
📋 Alternative: Use Zod for proper validation
In the controller or validation layer, define and apply a schema:
const evaluationScoresSchema = z.record(
z.string(),
z.object({
score: z.number(),
comment: z.string().optional(),
})
);Then validate before calling the service:
const parsed = evaluationScoresSchema.safeParse(evaluationScores);
if (!parsed.success) {
return res.status(400).json({ message: "Invalid evaluation scores", errors: parsed.error });
}This properly catches malformed input at the boundary rather than attempting to handle it in the service layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/src/module/recruiter/recruiter.service.ts` around lines 477 - 485, The
current catch in recruiter.service.ts using
JSON.parse(JSON.stringify(evaluationScores)) doesn't validate client input;
replace this with a proper schema validation at the request boundary (or inside
the service) for evaluationScores: define a Zod (or Joi) schema that describes
the expected record shape (keys -> { score: number, comment?: string }),
validate evaluationScores (e.g., safeParse) and on failure throw or return the
existing 422 error with validation details; remove the
JSON.parse(JSON.stringify(...)) deep-clone and use the validated value as
parsedScores so downstream code uses a type-safe, validated object.
1 participant
Pull Request
Description
Added a
Math.min(limit, 50)guard directly in thesearchTalentservice method inrecruiter.service.tsso the DB query is always capped at 50 records, independent of the Zod validation middleware.Related Issue
Fixes #1118
Type of Change
Testing
cd server && npm run dev/api/recruiter/talent?limit=9999&page=1with a recruiter JWT tokenpagination.limitin response is50, not9999npx tsc --noEmitnow passes with 0 errors (pre-existing prisma import error inopensource.routes.tsalso fixed)Screenshots / Video
No UI changes in this PR (delete this line if you are making UI changes)
Checklist
.env, credentials, ornode_modulescommittedSummary by CodeRabbit
Bug Fixes
Performance & Stability