fix(opensource): trim search query before database lookup#1218
fix(opensource): trim search query before database lookup#1218mayank200529 wants to merge 6 commits into
Conversation
github-actions
Bot
added
bug
|
Hi @mayank200529, 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
level:beginner
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughService ChangesWhitespace-trimmed optional search & route removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
|
I have completed the fix and submitted a PR for this issue. The implementation trims search input before processing and prevents whitespace-only queries from triggering unnecessary database lookups. Thank you for reviewing. |
github-actions
Bot
added
level:intermediate
|
I have completed the fix and submitted a PR for this issue. The implementation trims search input before processing and prevents whitespace-only queries from triggering unnecessary database lookups. Thank you for reviewing. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/src/module/opensource/opensource.routes.ts (2)
27-59: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove
/requests/minehandler logic to controller for consistency.Lines 27-59 contain inline Prisma queries and business logic directly in the route handler, while all other routes in this file delegate to
OpensourceControllermethods. This breaks the established layering pattern and violates the coding guideline that service layers should contain database queries.Move this logic to a controller method (e.g.,
controller.getMyRepoRequests) to maintain architectural consistency and improve testability.♻️ Suggested refactoring
In the routes file:
// Get my repo requests -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) +);Then add the corresponding method to
OpensourceController.🤖 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 27 - 59, The route handler for opensourceRouter.get("/requests/mine") contains inline DB/business logic (prisma.repoRequest, prisma.opensourceRepo and the approvedUrls/repoIdByUrl mapping) and should be moved into a controller method to match the rest of the file; create a method on OpensourceController (e.g., getMyRepoRequests) that accepts the current user (or req) and performs the same prisma.repoRequest.findMany, prisma.opensourceRepo.findMany, mapping to repoId, and returns the enriched requests, then replace the inline handler body with a call/delegation to OpensourceController.getMyRepoRequests(...)/its returned result and let the route only handle auth/role and response handling (res.json or next(err)).
11-86:⚠️ Potential issue | 🟠 MajorAdd
usageLimit(action)middleware (or document an explicit exemption) for opensource routes
server/src/module/opensource/opensource.routes.tsregisters/requests,/requests/mine,/requests/all, and/requests/:id/approve|rejectwithoutusageLimit(action)in the middleware chain. Coding guidelines require enforcing daily usage limits viausageLimit(action)with actionsATS_SCORE,COVER_LETTER,GENERATE_RESUME,JOB_APPLICATION, andMOCK_INTERVIEW; either applyusageLimit(...)to the relevant endpoints or justify why opensource functionality is exempt.🤖 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 11 - 86, Routes under opensourceRouter (notably the student-authenticated POST "/requests" -> submitRepoRequest, GET "/requests/mine", and GET "/analytics/trend" -> getStudentContributionTrend) are missing the usageLimit(action) middleware required by coding guidelines; either add the usageLimit middleware with the appropriate action constant(s) (e.g., usageLimit(ATS_SCORE) / usageLimit(COVER_LETTER) / usageLimit(GENERATE_RESUME) / usageLimit(JOB_APPLICATION) / usageLimit(MOCK_INTERVIEW)) into the middleware chain for these endpoints or add a concise exemption comment at the top of opensource.routes.ts explaining why opensource routes are excluded, referencing the usageLimit function and the listed actions so reviewers can verify compliance. Ensure you update both the inline route for "/requests/mine" and the router registrations that call controller.submitRepoRequest and controller.getStudentContributionTrend (and admin request handlers if they should be limited) so the middleware ordering matches authMiddleware and requireRole.
🤖 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.
Outside diff comments:
In `@server/src/module/opensource/opensource.routes.ts`:
- Around line 27-59: The route handler for
opensourceRouter.get("/requests/mine") contains inline DB/business logic
(prisma.repoRequest, prisma.opensourceRepo and the approvedUrls/repoIdByUrl
mapping) and should be moved into a controller method to match the rest of the
file; create a method on OpensourceController (e.g., getMyRepoRequests) that
accepts the current user (or req) and performs the same
prisma.repoRequest.findMany, prisma.opensourceRepo.findMany, mapping to repoId,
and returns the enriched requests, then replace the inline handler body with a
call/delegation to OpensourceController.getMyRepoRequests(...)/its returned
result and let the route only handle auth/role and response handling (res.json
or next(err)).
- Around line 11-86: Routes under opensourceRouter (notably the
student-authenticated POST "/requests" -> submitRepoRequest, GET
"/requests/mine", and GET "/analytics/trend" -> getStudentContributionTrend) are
missing the usageLimit(action) middleware required by coding guidelines; either
add the usageLimit middleware with the appropriate action constant(s) (e.g.,
usageLimit(ATS_SCORE) / usageLimit(COVER_LETTER) / usageLimit(GENERATE_RESUME) /
usageLimit(JOB_APPLICATION) / usageLimit(MOCK_INTERVIEW)) into the middleware
chain for these endpoints or add a concise exemption comment at the top of
opensource.routes.ts explaining why opensource routes are excluded, referencing
the usageLimit function and the listed actions so reviewers can verify
compliance. Ensure you update both the inline route for "/requests/mine" and the
router registrations that call controller.submitRepoRequest and
controller.getStudentContributionTrend (and admin request handlers if they
should be limited) so the middleware ordering matches authMiddleware and
requireRole.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52a1db62-b1b1-47e1-85a7-07e6dc042954
📒 Files selected for processing (2)
server/src/module/opensource/opensource.routes.tsserver/src/module/opensource/opensource.service.ts
mayank200529
force-pushed
the
fix-search-trim
branch
from
0f6a737 to
fe036cf
Compare
github-actions
Bot
added
level:beginner
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/opensource/opensource.service.ts`:
- Around line 24-30: The listRepos implementation uses the raw opts.search
(variable search) to build where.OR causing whitespace-only strings to bypass
the guard; update listRepos to mirror the getGsocOrgs pattern by trimming and
validating opts.search before using it (e.g., derive search =
opts.search?.trim() or similar) and only build where.OR when that trimmed search
is non-empty, so predicates like name/owner/description/tags are not created for
blank input.
🪄 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: 87015bff-1b4c-4ca0-8883-86558c991d87
📒 Files selected for processing (1)
server/src/module/opensource/opensource.service.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 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/opensource/opensource.service.ts`:
- Around line 24-30: The listRepos implementation uses the raw opts.search
(variable search) to build where.OR causing whitespace-only strings to bypass
the guard; update listRepos to mirror the getGsocOrgs pattern by trimming and
validating opts.search before using it (e.g., derive search =
opts.search?.trim() or similar) and only build where.OR when that trimmed search
is non-empty, so predicates like name/owner/description/tags are not created for
blank input.
🪄 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: 87015bff-1b4c-4ca0-8883-86558c991d87
📒 Files selected for processing (1)
server/src/module/opensource/opensource.service.ts
🛑 Comments failed to post (1)
server/src/module/opensource/opensource.service.ts (1)
24-30:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply the same trim-and-guard pattern to
listReposas used ingetGsocOrgs.The PR objectives explicitly state: "Apply the same trimming and validation in server/src/module/opensource/opensource.service.ts (listRepos) where opts.search is used." However,
listReposstill uses rawsearchwithout trimming. Whitespace-only strings like" "will pass theif (search)check at line 24 and build OR predicates with meaningless search terms, triggering unnecessary database queries—the exact bug this PR claims to fix.Apply the same pattern implemented in
getGsocOrgs(lines 62-69) to ensure consistent behavior and complete the fix.🔧 Proposed fix
async listRepos(query: any) { const { page, limit, search, language, difficulty, domain, sortBy, sortOrder } = query; const skip = (page - 1) * limit; const where: any = {}; if (language) where.language = language; if (difficulty) where.difficulty = difficulty; if (domain) where.domain = domain; - if (search) { + + const trimmedSearch = search?.trim(); + if (trimmedSearch) { where.OR = [ - { name: { contains: search, mode: "insensitive" } }, - { owner: { contains: search, mode: "insensitive" } }, - { description: { contains: search, mode: "insensitive" } }, - { tags: { hasSome: [search] } }, + { name: { contains: trimmedSearch, mode: "insensitive" } }, + { owner: { contains: trimmedSearch, mode: "insensitive" } }, + { description: { contains: trimmedSearch, mode: "insensitive" } }, + { tags: { hasSome: [trimmedSearch] } }, ]; }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if (language) where.language = language; if (difficulty) where.difficulty = difficulty; if (domain) where.domain = domain; const trimmedSearch = search?.trim(); if (trimmedSearch) { where.OR = [ { name: { contains: trimmedSearch, mode: "insensitive" } }, { owner: { contains: trimmedSearch, mode: "insensitive" } }, { description: { contains: trimmedSearch, mode: "insensitive" } }, { tags: { hasSome: [trimmedSearch] } }, ]; }🤖 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.service.ts` around lines 24 - 30, The listRepos implementation uses the raw opts.search (variable search) to build where.OR causing whitespace-only strings to bypass the guard; update listRepos to mirror the getGsocOrgs pattern by trimming and validating opts.search before using it (e.g., derive search = opts.search?.trim() or similar) and only build where.OR when that trimmed search is non-empty, so predicates like name/owner/description/tags are not created for blank input.
|
I have resolved the merge conflict and addressed the CodeRabbit feedback by using the trimmed search value consistently across the filters. The PR now has no conflicts and can be cleanly merged. Please let me know if any further changes are required. Thank you! |
github-actions
Bot
added
quality:clean
There was a problem hiding this comment.
Actionable comments posted: 3
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.service.ts (1)
184-184:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix indentation inconsistency.
Extra leading spaces before
if (trimmedSearch)— should align with surrounding code at 4 spaces.🧹 Proposed fix
- if (trimmedSearch) { + if (trimmedSearch) {🤖 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.service.ts` at line 184, The line with "if (trimmedSearch)" has extra leading spaces causing indentation inconsistency in OpensourceService; locate the occurrence inside the method that uses the trimmedSearch variable and align its indentation to 4 spaces to match the surrounding block (adjust preceding whitespace so the "if (trimmedSearch)" line and its enclosed block line up with other statements in that scope).
🤖 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/opensource/opensource.routes.ts`:
- Around line 28-48: There is an unresolved merge conflict around the Student
Progress Tracking routes; accept the upstream/main changes by removing the
conflict markers and keeping the added routes: ensure
opensourceRouter.get("/first-pr/progress", authMiddleware,
requireRole("STUDENT", ...)) calling controller.getFirstPrProgress and
opensourceRouter.patch("/first-pr/progress", authMiddleware,
requireRole("STUDENT", ...)) calling controller.patchFirstPrProgress remain in
the file and are placed before any "/:id" or "/requests/*" routes to avoid route
conflicts; remove the leftover <<<<<<<, =======, and >>>>>>> markers so the file
compiles cleanly.
- Around line 1-6: Remove the unresolved Git conflict markers (<<<<<<<, =======,
>>>>>>>) and the BOM character before the first import, and restore the upstream
import of prisma so the route handler that queries the database (the
/requests/mine route which uses prisma) compiles; specifically, keep a single
clean import block including: import { Router } from "express" and import {
prisma } from "../../database/db.js", then verify the route handler referencing
prisma (in the /requests/mine handler) uses that imported prisma instance.
In `@server/src/module/opensource/opensource.service.ts`:
- Around line 81-117: Remove the Git conflict markers (<<<<<<<, =======,
>>>>>>>) and resolve the merge by keeping the HEAD variant that consistently
uses trimmedSearch for all predicates; specifically ensure the block that runs
prisma.$queryRaw to compute tagMatches, maps to tagMatchIds, and builds
where["OR"] uses { name: { contains: trimmedSearch, mode: "insensitive" } }, {
owner: { contains: trimmedSearch, mode: "insensitive" } }, { description: {
contains: trimmedSearch, mode: "insensitive" } }, { language: { contains:
trimmedSearch, mode: "insensitive" } } and conditionally includes { id: { in:
tagMatchIds } } when tagMatchIds is non-empty (preserve tagMatches, tagMatchIds,
trimmedSearch, and where["OR"] identifiers).
---
Outside diff comments:
In `@server/src/module/opensource/opensource.service.ts`:
- Line 184: The line with "if (trimmedSearch)" has extra leading spaces causing
indentation inconsistency in OpensourceService; locate the occurrence inside the
method that uses the trimmedSearch variable and align its indentation to 4
spaces to match the surrounding block (adjust preceding whitespace so the "if
(trimmedSearch)" line and its enclosed block line up with other statements in
that scope).
🪄 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: 2baafbaa-f3bd-443d-a092-5ae3becaa1cd
📒 Files selected for processing (2)
server/src/module/opensource/opensource.routes.tsserver/src/module/opensource/opensource.service.ts
| <<<<<<< HEAD | ||
| import { Router } from "express"; | ||
| ======= | ||
| import { Router } from "express"; | ||
| import { prisma } from "../../database/db.js"; | ||
| >>>>>>> upstream/main |
There was a problem hiding this comment.
Unresolved merge conflict markers break compilation.
The file contains unresolved Git conflict markers. Additionally, the HEAD side has a BOM (Byte Order Mark) character () before the import statement which should be removed.
The prisma import from upstream/main is required for the /requests/mine route (line 57).
🔧 Proposed resolution
-<<<<<<< HEAD
-import { Router } from "express";
-=======
import { Router } from "express";
import { prisma } from "../../database/db.js";
->>>>>>> upstream/main📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <<<<<<< HEAD | |
| import { Router } from "express"; | |
| ======= | |
| import { Router } from "express"; | |
| import { prisma } from "../../database/db.js"; | |
| >>>>>>> upstream/main | |
| import { Router } from "express"; | |
| import { prisma } from "../../database/db.js"; |
🧰 Tools
🪛 Biome (2.4.16)
[error] 1-1: Expected a statement but instead found '<<<<<<< HEAD'.
(parse)
[error] 3-3: Expected a statement but instead found '======='.
(parse)
[error] 6-6: Expected a statement but instead found '>>>>>>> upstream/main'.
(parse)
🤖 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 1 - 6, Remove
the unresolved Git conflict markers (<<<<<<<, =======, >>>>>>>) and the BOM
character before the first import, and restore the upstream import of prisma so
the route handler that queries the database (the /requests/mine route which uses
prisma) compiles; specifically, keep a single clean import block including:
import { Router } from "express" and import { prisma } from
"../../database/db.js", then verify the route handler referencing prisma (in the
/requests/mine handler) uses that imported prisma instance.
| @@ -39,6 +45,7 @@ opensourceRouter.patch( | |||
| ); | |||
|
|
|||
| // ─── Repo Requests (Student-authenticated) ───────────────────── | |||
| >>>>>>> upstream/main | |||
There was a problem hiding this comment.
Unresolved merge conflict in Student Progress Tracking section.
Resolve this conflict by accepting the upstream/main side which adds the new /first-pr/progress routes. These routes are correctly placed before /:id to avoid route conflicts, per coding guidelines.
🔧 Proposed resolution
-<<<<<<< HEAD
-=======
// ─── Student Progress Tracking ─────────────────────────────────
// NOTE: must be before /:id to avoid route conflicts
opensourceRouter.get(
"/first-pr/progress",
authMiddleware,
requireRole("STUDENT"),
(req, res, next) => controller.getFirstPrProgress(req, res, next),
);
opensourceRouter.patch(
"/first-pr/progress",
authMiddleware,
requireRole("STUDENT"),
(req, res, next) => controller.patchFirstPrProgress(req, res, next),
);
// ─── Repo Requests (Student-authenticated) ─────────────────────
->>>>>>> upstream/mainBased on learnings: /requests/* routes must appear BEFORE /:id to avoid route conflicts.
🧰 Tools
🪛 Biome (2.4.16)
[error] 28-38: Expected a statement but instead found '<<<<<<< HEAD
// ─── Student Progress Tracking ─────────────────────────────────
// NOTE: must be before /:id to avoid route conflicts
opensourceRouter.get(
"/first-pr/progress",
authMiddleware,
requireRole("STUDENT"),
(req, res, next) => controller.getFirstPrProgress(req, res, next),
)'.
(parse)
🤖 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 28 - 48,
There is an unresolved merge conflict around the Student Progress Tracking
routes; accept the upstream/main changes by removing the conflict markers and
keeping the added routes: ensure opensourceRouter.get("/first-pr/progress",
authMiddleware, requireRole("STUDENT", ...)) calling
controller.getFirstPrProgress and opensourceRouter.patch("/first-pr/progress",
authMiddleware, requireRole("STUDENT", ...)) calling
controller.patchFirstPrProgress remain in the file and are placed before any
"/:id" or "/requests/*" routes to avoid route conflicts; remove the leftover
<<<<<<<, =======, and >>>>>>> markers so the file compiles cleanly.
|
I investigated the failing The failure does not appear to be caused by this PR specifically. Running the server typecheck reports a large number of existing TypeScript/Prisma-related errors across many unrelated modules in the repository (hundreds of errors across dozens of files), including modules that were not modified by this PR. The changes in this PR are limited to the Open Source module and the intended search-trimming fix has been implemented successfully. Could you please confirm whether the current typecheck failure is due to existing issues on the base branch? I’m happy to make any PR-specific adjustments if needed. Thank you! |
1 participant
Pull Request
Description
Fixed whitespace-only search queries triggering unnecessary database lookups in the Open Source module.
Changes
opensource.routes.tsto trim search input before processing.opensource.service.tsto trim search input before applying filters.Related Issue
Fixes #1000
Type of Change
Testing
Tested the following scenarios:
?search=react) works correctly.?search= react) is trimmed and works correctly.?search=) no longer triggers unnecessary database filtering.Screenshots / Video
No UI changes in this PR
Checklist
.env, credentials, ornode_modulescommittedSummary by CodeRabbit
Bug Fixes
Chores