fix: ProtectedRoute toast on role mismatch (#2257), pass email via state (#2356) - #1
Open
saurabhhhcodes wants to merge 49 commits into
Open
fix: ProtectedRoute toast on role mismatch (#2257), pass email via state (#2356)#1saurabhhhcodes wants to merge 49 commits into
saurabhhhcodes wants to merge 49 commits into
Conversation
…ollow-ups (Sachinchaurasiya360#2200) Migrate the Express backend to run on Vercel as a single serverless function. - Wrap the app as an importable module with a VERCEL guard so EC2/local still boot the listener + node-cron; Vercel imports the configured app (api/index.ts). - Extract run-once functions from every cron and expose two consolidated daily Vercel Cron endpoints: /api/cron/daily (fast jobs) and /api/cron/pipeline (slow jobs), bearer-authed via CRON_SECRET, sequential with a soft time budget and per-job status. - vercel.json: catch-all rewrite to the function, maxDuration, and the two daily crons. - db pool size now reads PG_POOL_MAX (low default on Vercel). - tsconfig.api.json wires api/ into typecheck. - Includes the ambassador post-merge follow-ups from Sachinchaurasiya360#2141 review. - DB dump and migration script kept out of history (gitignored).
…unauthenticated users (Sachinchaurasiya360#2262) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
… ARIA compliance (Sachinchaurasiya360#2263) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…eld value (Sachinchaurasiya360#2266) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…achinchaurasiya360#2268) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…tory path (Sachinchaurasiya360#2269) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…360#2270) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…a360#2271) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…2280) Signed-off-by: Akshita-2307 <akshita@example.com> Co-authored-by: Akshita-2307 <akshita@example.com>
…age (Sachinchaurasiya360#2326) (Sachinchaurasiya360#2344) Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…nchaurasiya360#2334) Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…) (Sachinchaurasiya360#2337) Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…inchaurasiya360#2325) (Sachinchaurasiya360#2342) Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…asiya360#2330) (Sachinchaurasiya360#2341) Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…aurasiya360#2250) The GET /:id route was registered before POST /trigger, which is fragile - any future GET /trigger would be silently shadowed by /:id. Moved the dynamic route to the end, after all static routes, with a comment noting the ordering requirement. Closes Sachinchaurasiya360#2240 Signed-off-by: Xenon010101 <xenon010101@users.noreply.github.com> Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…2249) The POST /logout route was missing authMiddleware unlike every other user-specific endpoint in the auth module. While clearing the cookie is low-impact currently, this inconsistency could become a real issue if logout logic expands to include server-side side effects. Closes Sachinchaurasiya360#2239 Signed-off-by: Xenon010101 <xenon010101@users.noreply.github.com> Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…achinchaurasiya360#2248) The clientErrors map matched errors by exact err.message string, so minor variations in message phrasing would fall through to a generic 500. Added keyword-based fallback matching: 'not found' -> 404, 'already exists/registered/applied' -> 409, 'unauthorized/not authorized' -> 403. Closes Sachinchaurasiya360#2238 Signed-off-by: Xenon010101 <xenon010101@users.noreply.github.com> Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…asiya360#2244) The route was defined as '/learn/readiness' but the router is mounted at '/api/learn', making the actual path '/api/learn/learn/readiness'. Changed to '/readiness' so the intended path '/api/learn/readiness' works correctly. Closes Sachinchaurasiya360#2234 Signed-off-by: Xenon010101 <xenon010101@users.noreply.github.com> Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…a360#2331) (Sachinchaurasiya360#2338) Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…essionStorage for chunk reload retry (Sachinchaurasiya360#2343) * fix: use module-level variable instead of sessionStorage for chunk reload retry (Sachinchaurasiya360#2328) * fix: move render-phase state mutations into useEffect on SqlExercisePage (Sachinchaurasiya360#2326) --------- Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…a360#2246) When AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, or AWS_S3_BUCKET env vars were missing, the S3 client silently used empty-string fallbacks causing cryptic AWS auth errors at runtime. Added descriptive startup validation that throws immediately on misconfiguration. Closes Sachinchaurasiya360#2236 Signed-off-by: Xenon010101 <xenon010101@users.noreply.github.com> Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…iption activation (Sachinchaurasiya360#2346) * fix: move render-phase state mutations into useEffect on SqlExercisePage (Sachinchaurasiya360#2326) * fix: handle out-of-order webhooks in subscription activation (Sachinchaurasiya360#2323) --------- Co-authored-by: Xenon010101 <xenon010101@users.noreply.github.com>
…Sachinchaurasiya360#2371) Switch express-rate-limit to its named export and enable esModuleInterop so helmet's default import is callable under Vercel's CJS module resolution. Fixes the TS2349 build failures.
Sachinchaurasiya360#2372) esModuleInterop did not make helmet callable on Vercel; load it via createRequire (always the CJS callable at runtime) and type it from the type-only HelmetOptions export.
…360#2345) - Refactored roadmap.service to isolate static structure queries from dynamic user progress - Cached published roadmap structures by slug using the shared caching layer - Implemented targeted cache invalidations for updates, regenerations, and status changes - Maintained strict visibility boundary checks for private AI-generated paths Closes Sachinchaurasiya360#2311
…nchaurasiya360#2322) - Wrapped inside a try/catch block - Implemented a temporary textarea DOM fallback for environments with blocked permissions - Added descriptive success/error toast notifications depending on the copy result Closes Sachinchaurasiya360#2207
…hinchaurasiya360#2224) * fix: replace hardcoded ambassador eligibility tresholds with named constants * fix: refetch submissions on window focus to prevent stale counts
…ses runtime error (Sachinchaurasiya360#2287) * feat: create ApplicationsList component for recruiters to view and manage job applications * typecheck fixes * removing unwanted..imports
…achinchaurasiya360#2291) * feat: Add dismissible keyboard navigation hint for guide sections * fix: use shared Button component for shortcut hint dismissal
…achinchaurasiya360#2320) * test(opensource): add unit tests for learning path context tracking * test(opensource): bypass lifecycle timing bugs using absolute state mock implementation matching * test(opensource): solve eslint ban-ts-comment error and drop explicit any types --------- Co-authored-by: Sameeksha Katiyar <zara812singh@gmail.com>
…+1 queries (Sachinchaurasiya360#2201) (Sachinchaurasiya360#2222) * perf: optimize ambassador eligibility cron and service to eliminate N+1 queries (Sachinchaurasiya360#2201) - Refactored to replace the N+1 loop with a 4-step progressive batch-filtering pipeline. - Utilized with for single-pass leaderboard ranking. - Updated to run standalone eligibility checks concurrently using . - Ensures zero database read operations inside the cron execution loop. * fix(client): strongly type any occurrences to fix lint warnings (Sachinchaurasiya360#2201) * chore: sync package-lock.json with package changes --------- Co-authored-by: Sachin Chaurasiya <mrsachinchaurasiya@gmail.com>
express-rate-limit v8's default export resolves to a non-callable module namespace when the package is resolved through its CJS type condition, which happened on Vercel's fresh install but not on cached local installs. Switch all imports to the named `rateLimit`/`ipKeyGenerator` exports, which are plain callable consts in every type variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Same CJS/ESM interop issue as express-rate-limit: dodopayments' default export resolves to a non-callable/non-type module namespace when the package resolves through its CJS condition on Vercel. Use the named DodoPayments class export, which works as both a type and a constructor in every variant. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both are Stainless-generated SDKs with the same CJS/ESM interop issue as dodopayments: the default export resolves to a non-callable/non-type module namespace under the package's CJS type condition (which Vercel resolves to, local resolves to the ESM .d.mts). Use the named Groq/OpenAI class exports, which are constructable and usable as a type under both conditions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The serverless function crashed with ERR_REQUIRE_ESM: html-encoding-sniffer@6 (via jsdom -> isomorphic-dompurify) require()s the ESM-only @exodus/bytes/encoding-lite.js. @exodus/bytes targets Node ^20.19 || ^22.12 || >=24, the versions where require(ESM) is supported. Vercel was defaulting to an older Node that cannot require() an ESM module. Pin engines.node to 22.x so the build and function runtime use a Node that supports require(ESM). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi @saurabhhhcodes, 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
quality:clean
📝 WalkthroughWalkthroughThis PR introduces Vercel serverless deployment support for the Express server, including HTTP-invoked cron endpoints replacing long-running schedulers, standardized exported ChangesVercel Deployment and Cron Infrastructure
Feature Improvements and Bug Fixes
Sequence Diagram(s)sequenceDiagram
participant Vercel Scheduler
participant cronRouter
participant runGroup
participant runOneJob
participant CronWorker
participant withAdvisoryLock
Vercel Scheduler->>cronRouter: GET /api/cron/daily (Bearer token)
cronRouter->>cronRouter: validate CRON_SECRET
cronRouter->>runGroup: LIGHT_JOBS, utcDay
loop each job (sequential)
runGroup->>runGroup: check onlyOnUtcDay, soft budget
runGroup->>runOneJob: job
runOneJob->>withAdvisoryLock: acquire lock
withAdvisoryLock->>CronWorker: run*()
CronWorker-->>withAdvisoryLock: done/error
withAdvisoryLock-->>runOneJob: release lock
runOneJob-->>runGroup: JobResult (ok/error + timing)
end
runGroup-->>cronRouter: JobResult[]
cronRouter-->>Vercel Scheduler: 200 or 207 JSON
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 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 docstrings
🧪 Generate unit tests (beta)
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: 9
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)
433-450:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap repo creation and request update in a transaction to prevent data inconsistency.
If the
opensourceRepo.createsucceeds but the subsequentrepoRequest.updatefails (network error, DB constraint, etc.), the request remainsPENDINGwithrepoId: null. An admin retrying the approval would create a duplicate repository for the same request, since the status check on line 431 would still pass.🔒 Proposed fix
- const repo = await prisma.opensourceRepo.create({ - data: { - name: overrides.name ?? request.name, - owner: request.owner, - description: overrides.description ?? request.description, - language: request.language, - url: request.url, - domain: (overrides.domain ?? request.domain) as RepoDomain, - difficulty: (overrides.difficulty ?? request.difficulty) as RepoDifficulty, - techStack: request.techStack, - tags: overrides.tags ?? request.tags, - }, - }); - - await prisma.repoRequest.update({ - where: { id }, - data: { status: "APPROVED", adminNote: overrides.adminNote ?? null, repoId: repo.id }, - }); + const repo = await prisma.$transaction(async (tx) => { + const repo = await tx.opensourceRepo.create({ + data: { + name: overrides.name ?? request.name, + owner: request.owner, + description: overrides.description ?? request.description, + language: request.language, + url: request.url, + domain: (overrides.domain ?? request.domain) as RepoDomain, + difficulty: (overrides.difficulty ?? request.difficulty) as RepoDifficulty, + techStack: request.techStack, + tags: overrides.tags ?? request.tags, + }, + }); + + await tx.repoRequest.update({ + where: { id }, + data: { status: "APPROVED", adminNote: overrides.adminNote ?? null, repoId: repo.id }, + }); + + return repo; + });🤖 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 433 - 450, The opensourceRepo.create and repoRequest.update operations are performed sequentially without transactional guarantees, which can cause data inconsistency if the first operation succeeds but the second fails. Wrap both the prisma.opensourceRepo.create call and the subsequent prisma.repoRequest.update call within a prisma.$transaction block to ensure both operations are atomic, preventing the scenario where a repo is created but the request status is not updated, which would lead to duplicate repos if an admin retries the approval.
🧹 Nitpick comments (12)
server/src/__tests__/leetcode.service.test.ts (1)
129-168: 💤 Low valueMisleading test description.
The test description says "keep their original solve dates" but the assertion on line 164 verifies that
solvedAtis updated to the LeetCode timestamp (new Date(1672531200 * 1000)), not the original date. Consider renaming to clarify the actual behavior:- it("should update existing records that are NOT solved, and keep their original solve dates", async () => { + it("should update existing unsolved records with the LeetCode solve timestamp", async () => {🤖 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/__tests__/leetcode.service.test.ts` around lines 129 - 168, The test description for the test case starting with "should update existing records that are NOT solved, and keep their original solve dates" is misleading because the actual assertion on line 164 verifies that solvedAt is updated to the LeetCode timestamp (new Date(1672531200 * 1000)), not the original date. Update the test description string in the it() function to accurately reflect the actual behavior being tested, such as clarifying that the solve dates are set from LeetCode submission timestamps rather than being preserved.server/src/__tests__/opensource.service.test.ts (1)
70-91: ⚡ Quick winInclude
userfield in test mock to properly validate the email flow.The service's
approveRepoRequestmethod includes{ user: { select: { name: true, email: true } } }in thefindUniquecall (line 428 of the service), but the test'smakeRequesthelper and the mock on line 166 don't include this field. When the service attempts to send an email usingrequest.user.emailandrequest.user.name(service lines 454–460), it receivesundefinedvalues. Although the mockedsendEmaildoesn't fail, the test doesn't validate that the correct data structure flows through the email path.♻️ Suggested enhancement
function makeRequest(overrides: Record<string, unknown> = {}) { return { id: REQUEST_ID, name: "test-repo", owner: "test-owner", description: "A test repository", language: "TypeScript", url: "https://github.com/test-owner/test-repo", domain: "WEB", difficulty: "BEGINNER", techStack: [], tags: [], reason: "For learning", status: "PENDING", adminNote: null, userId: USER_ID, repoId: null, + user: { + name: "Test User", + email: "test@example.com", + }, createdAt: new Date(), updatedAt: new Date(), ...overrides, } as any; }Also applies to: 164-177
🤖 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/__tests__/opensource.service.test.ts` around lines 70 - 91, The makeRequest helper function does not include a user field, but the service's approveRepoRequest method attempts to access request.user.email and request.user.name when sending emails. Add a user field to the object returned by makeRequest with name and email properties to match the data structure returned by the service's findUnique query with the user select clause. Also update the mock on lines 164-177 to include the same user field structure so the email sending flow is properly validated.server/src/module/learn/learn.service.ts (1)
161-161: ⚡ Quick winPrefer structured logging over console.error.
The codebase includes a logger utility (referenced in graph context at
server/src/utils/logger.ts) with structured error logging. Usingconsole.errorbypasses centralized log management, observability integrations, and consistent formatting.♻️ Recommended change
+import { logger } from "../../utils/logger.js"; + export class LearnService { async calculateReadinessReport(data: { // ... }) { // ... } catch (error) { - console.error("Gemini exception in readiness report, using fallback:", error); + logger.error("Gemini exception in readiness report, using fallback", error); // Clean safety fallback so the server never crashes even if API keys are missing locally🤖 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/learn/learn.service.ts` at line 161, Replace the console.error call in the Gemini exception handling block with the structured logger utility from server/src/utils/logger.ts. Import the logger at the top of the learn.service.ts file and replace the console.error statement that logs "Gemini exception in readiness report, using fallback:" with the appropriate logger method to maintain consistent, structured error logging across the codebase.server/src/__tests__/learn.service.test.ts (1)
104-104: ⚡ Quick winConsider extracting the fallback calculation to avoid test coupling.
The test hardcodes the fallback formula (
completedCount * 10 + dsaCount * 5), which tightly couples the test to the implementation details. If the formula changes, both the service and test must be updated in sync. Consider extracting the calculation logic to a separate pure function that can be tested independently, making the main test verify the function is called rather than duplicating its logic.♻️ Recommended refactoring approach
In the service:
// Extract calculation to a testable pure function export function calculateFallbackReadiness( completedLessons: number, dsaSolved: number ): number { return Math.min(100, Math.max(10, completedLessons * 10 + dsaSolved * 5)); } // Use it in the service return { overallReadiness: calculateFallbackReadiness(completedLessonsCount, totalDsaSolved), // ... };In the test:
// Now test just verifies the function is used, not the formula expect(result.overallReadiness).toBe( calculateFallbackReadiness(2, 5) );Add a separate test file for
calculateFallbackReadinessto verify the formula logic independently.🤖 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/__tests__/learn.service.test.ts` at line 104, Extract the fallback readiness calculation formula (completedCount * 10 + dsaCount * 5 with Math.min and Math.max bounds) from the test into a separate pure function called calculateFallbackReadiness in the service file. Update the service implementation to use this extracted function, then modify the test expectation in the describe block to call calculateFallbackReadiness(2, 5) instead of hardcoding the formula calculation. Finally, create a dedicated test file to thoroughly test the calculateFallbackReadiness function independently to verify the formula logic and edge cases.server/src/module/auth/auth.routes.ts (1)
69-69: 💤 Low valueLogout now requires authentication.
The
/logoutendpoint now requiresauthMiddleware, meaning unauthenticated users will receive a 401 response. This is appropriate if the logout logic needs to identify the user (e.g., to invalidate specific tokens), but differs from typical logout endpoints that accept unauthenticated requests.Verify this aligns with your session invalidation strategy.
🤖 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/auth/auth.routes.ts` at line 69, The logout endpoint now requires authMiddleware, which means only authenticated users can logout. Verify that the authController.logout() method's session invalidation logic actually requires user identification. Check if the logout implementation (the logout method in authController) uses the authenticated user information from the request to invalidate tokens or sessions. If the logout logic does require user context, the authMiddleware requirement is correct; if not, consider removing the authMiddleware parameter from the authRouter.post() route definition to allow unauthenticated logout requests.server/src/middleware/error.middleware.ts (1)
157-170: 💤 Low valueConsider the risk of false positives in substring matching.
The fallback substring matching (case-insensitive) may assign incorrect status codes when error messages contain the target substrings in unexpected contexts. For example, an error message like
"Failed to process: 'not found' is not a valid status"would be misclassified as 404.However, since this fallback runs only after exact matches fail and covers common dynamic error patterns, the practical risk is low.
🤖 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/middleware/error.middleware.ts` around lines 157 - 170, The fallback substring matching logic using msg.includes() checks for patterns like "not found", "already exists/registered/applied", and "unauthorized" can produce false positives when these substrings appear in unexpected contexts within error messages. To fix this, replace the simple substring matching with more context-aware pattern matching using regular expressions with word boundaries (like /\bnot found\b/i for the "not found" check) to ensure the target words match as whole words rather than arbitrary substrings, thereby reducing the risk of misclassifying error messages that happen to contain these terms in unintended contexts.client/src/components/DynamicFieldRenderer.tsx (1)
28-72: ⚡ Quick winSynchronize local error state with parent value prop.
The
fileErrorstate is local toFileUploadFieldand only clears when the user selects a new file (line 44). If the parent component externally clears thevalueprop (e.g., form reset), the error message will persist without a selected file, creating confusing UX.♻️ Recommended fix to sync error state with value prop
function FileUploadField({ field, value, onChange, onFileSelect, disabled }: { field: CustomFieldDefinition; value: unknown; onChange: (val: unknown) => void; onFileSelect?: (file: File) => void; disabled?: boolean; }) { const [fileError, setFileError] = useState<string | null>(null); + + // Clear error when parent clears the value + useEffect(() => { + if (!value) { + setFileError(null); + } + }, [value]); + const maxSize = field.validation?.maxFileSize || 5 * 1024 * 1024;Note: Add
useEffectto the imports at the top of the file:-import { useState } from "react"; +import { useState, useEffect } from "react";🤖 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 `@client/src/components/DynamicFieldRenderer.tsx` around lines 28 - 72, The FileUploadField component maintains local fileError state that only clears when the user selects a new file, but if the parent component externally clears the value prop (such as during a form reset), the error message persists without a selected file, creating poor UX. Add a useEffect hook that monitors the value prop and automatically clears fileError by calling setFileError(null) whenever value becomes empty or falsy, ensuring the local error state stays synchronized with the parent value prop. Include value as a dependency in the useEffect dependency array.client/src/components/ProtectedRoute.tsx (1)
7-12: 💤 Low valueConsider adding
messageto the dependency array.The
useEffectreadsmessagebut omits it from the dependency array. While this won't cause runtime issues (the component unmounts immediately after the toast), it violates React Hooks rules and may trigger ESLint warnings.♻️ Recommended adjustment
function RedirectWithToast({ to, message }: { to: string; message?: string }) { useEffect(() => { toast.error(message || "Please login to access this resource"); - }, []); + }, [message]); return <Navigate to={to} replace />; }🤖 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 `@client/src/components/ProtectedRoute.tsx` around lines 7 - 12, The useEffect hook in the RedirectWithToast function reads the message prop in the toast.error call but omits it from the dependency array. Add message to the dependency array of the useEffect (changing [] to [message]) to comply with React Hooks rules and avoid ESLint warnings about missing dependencies.client/src/components/common/ScrollToTop.tsx (1)
12-15: ⚡ Quick winRemove
statefrom the dependency array to prevent unwanted scrolls.Including
statein theuseEffectdependencies means the effect will re-run whenever state changes, even ifpathnameremains the same. This can cause unexpected scroll-to-top behavior during state-only navigation updates.The intended behavior is: "scroll to top when the pathname changes, unless
scrollToTopis explicitlyfalse." Thestatevariable is already in scope within the effect, so the check on Line 13 will work correctly with[pathname]as the sole dependency.♻️ Recommended fix
useEffect(() => { if ((state as { scrollToTop?: boolean } | null)?.scrollToTop === false) return; window.scrollTo(0, 0); - }, [pathname, state]); + }, [pathname]);🤖 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 `@client/src/components/common/ScrollToTop.tsx` around lines 12 - 15, In the ScrollToTop component, remove state from the useEffect dependency array and keep only pathname. The effect currently re-runs whenever state changes due to state being listed as a dependency, but the intended behavior is to scroll to top only when pathname changes. Since the state variable is already accessible within the effect's scope, the condition checking for scrollToTop will still work correctly with just [pathname] as the dependency.client/src/module/student/opensource/components/GuideSectionPage.tsx (1)
88-95: ⚡ Quick winAvoid empty
catchin dismiss handler.Line 91 currently swallows storage errors silently with an empty block, which keeps the lint warning and hides intent. Add a no-op comment or lightweight fallback signal.
Suggested patch
const dismissShortcutHint = () => { try { localStorage.setItem("guide-hint-dismissed", "true"); } catch { - + // localStorage may be unavailable (private mode / blocked storage) } setShowShortcutHint(false); };🤖 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 `@client/src/module/student/opensource/components/GuideSectionPage.tsx` around lines 88 - 95, The dismissShortcutHint function has an empty catch block when calling localStorage.setItem that swallows errors silently and violates lint rules. Replace the empty catch block with either a no-op comment explaining the intent to gracefully ignore storage errors, or add a lightweight fallback such as a console warning or error log to signal the failure while still allowing the hint dismissal to proceed. Ensure the catch handler makes the intent explicit rather than leaving it empty.Source: Linters/SAST tools
server/vercel.json (1)
3-7: ⚖️ Poor tradeoffConsider whether 60-second maxDuration is sufficient for heavy jobs.
The
maxDuration: 60seconds applies to both/api/cron/daily(LIGHT_JOBS) and/api/cron/pipeline(HEAVY_JOBS). According to the context, HEAVY_JOBS includes network-bound scraping, AI embed/match passes, and per-user email loops—operations that can easily exceed 60 seconds.The soft budget of 55s (SOFT_BUDGET_MS) will skip jobs that don't fit, degrading gracefully, but this means heavy jobs may not complete on the Hobby tier. The comment in
daily-cron.route.tsacknowledges this: "~60s Hobby / ~300s Pro".This is acceptable if the team understands the trade-off and plans to upgrade to Pro tier when needed. However, skipped jobs will only retry on the next day's cron trigger, potentially causing 24-hour delays in scraping/AI processing.
Consider documenting this limitation in the vercel.json as a comment or in deployment documentation to make the tier constraint explicit for future maintainers.
🤖 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/vercel.json` around lines 3 - 7, The maxDuration setting of 60 seconds in the "api/index.ts" configuration may be insufficient for heavy jobs involving scraping and AI processing, but this limitation is not documented. Add a comment or documentation in vercel.json explaining that the 60-second limit applies to both light and heavy cron jobs, that heavy jobs may be skipped on the Hobby tier due to the 55-second soft budget, and that upgrading to Pro tier (which supports ~300 seconds) is recommended for heavier workloads. This will ensure future maintainers understand the tier-specific constraints and the trade-off that skipped jobs only retry the next day.server/src/cron/daily-cron.route.ts (1)
148-151: 💤 Low valueConsider timing-safe comparison for the Bearer token.
The string comparison using
!==is vulnerable to timing attacks. While the risk is low for cron secrets (limited attack surface, rate limiting by Vercel), using a constant-time comparison is a defense-in-depth measure.🛡️ Suggested fix using crypto.timingSafeEqual
+import { timingSafeEqual } from "crypto"; + +function safeCompare(a: string, b: string): boolean { + if (a.length !== b.length) return false; + return timingSafeEqual(Buffer.from(a), Buffer.from(b)); +} + // In makeCronHandler: - if (req.headers.authorization !== `Bearer ${secret}`) { + const expected = `Bearer ${secret}`; + const provided = req.headers.authorization ?? ""; + if (!safeCompare(provided, expected)) { res.status(401).json({ message: "Unauthorized" }); return; }🤖 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/cron/daily-cron.route.ts` around lines 148 - 151, The authorization header comparison in the route handler is using a standard string comparison with `!==` which is vulnerable to timing attacks. Replace this vulnerable comparison by importing crypto.timingSafeEqual from the Node.js crypto module and using it to perform a constant-time comparison between the Bearer token extracted from req.headers.authorization and the secret value. Ensure both values are converted to Buffers before passing them to timingSafeEqual for proper timing-safe comparison.
🤖 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 `@client/src/components/Navbar.tsx`:
- Line 421: The Navbar component at line 421 uses role="menuitem" which requires
complete keyboard support for ARIA menu patterns including arrow key navigation,
Enter to activate, and Escape to close. Since the menu items are functioning as
links in a mobile navigation pattern, remove the role="menuitem" attribute from
the menu item element and remove any parent role="menu" attributes if present.
This allows the components to use implicit link semantics which is more
appropriate for this use case and does not require additional keyboard event
handlers.
In `@client/src/components/ProtectedRoute.tsx`:
- Around line 31-33: VerifyEmailPage is currently reading the email from query
parameters using useSearchParams(), but ProtectedRoute is passing it via
location.state. In VerifyEmailPage, replace the useSearchParams() hook import
and usage with useLocation(), then access the email value from
location.state.email instead of from the search parameters. This will ensure the
email is properly received from the Navigate state passed by ProtectedRoute.
In `@server/api/index.ts`:
- Line 7: The catch block in the initServiceProviders() call is silently
swallowing all errors without any logging, which hides critical initialization
failures from operators. Replace the empty catch handler with a catch block that
logs the error details (using an appropriate logger) so visibility is maintained
into why AI features might fail at runtime, while still allowing the application
startup to continue.
In `@server/src/cron/job-cleanup.cron.ts`:
- Line 8: The RETAIN_DAYS constant assignment uses Number() to convert the
environment variable but does not validate that the result is a valid number. If
JOB_CLEANUP_RETAIN_DAYS is set to a non-numeric value, Number() will return NaN,
which will break the date calculation that uses RETAIN_DAYS. Add validation
after the constant assignment to check if RETAIN_DAYS is a valid number using
Number.isNaN() or isNaN(), and if it's not valid, either log a warning and use
the default value of 30, or throw an error to prevent invalid date calculations
downstream.
In `@server/src/module/ats/ats.validation.ts`:
- Around line 5-19: The isAllowedUrl function has a security vulnerability where
it allows any https URL when S3_BUCKET is not configured. The condition that
checks if S3_BUCKET is falsy and then returns true for any https URL is overly
permissive and could enable SSRF attacks or processing of untrusted resume
files. Replace this permissive check with a stricter validation that either
rejects the URL entirely (returns false) when S3_BUCKET is not configured, or at
minimum logs a warning indicating that production-grade validation is disabled
before proceeding.
In `@server/src/module/learn/learn.service.ts`:
- Around line 103-142: The user-controlled variables targetRole, companyTier,
and availableTime are directly interpolated into the systemPrompt template
literal without validation. Before constructing the system prompt, validate
these three fields against their allowed enumeration values or permitted
patterns. Add validation logic in the service method to ensure only safe,
expected values are accepted, and reject or sanitize any unexpected inputs. This
prevents malicious users from injecting arbitrary content into the AI prompt
that could manipulate the model's reasoning or cause unexpected behavior.
In `@server/src/module/payment/payment.service.ts`:
- Line 1: The import statement for the DodoPayments module is using incorrect
named import syntax with curly braces. Change the import statement at the top of
the payment.service.ts file from a named import to a default import by removing
the curly braces around DodoPayments, since the dodopayments v2.32.0 package
exports a default export rather than a named export.
- Around line 226-239: The placeholder payment created in tx.payment.create()
lacks a dodoPaymentId, but the payment update handler at lines 104-112 searches
for records using where: { dodoPaymentId: payment.checkout_session_id }. Since
the placeholder's dodoPaymentId is null, it will never be matched and will
remain orphaned in PENDING status. Implement a reconciliation approach by either
creating a periodic job that matches orphaned PENDING payments by
dodoSubscriptionId and userId together, then updates them when payment.succeeded
arrives, or modify the payment update logic in the handler to match using
dodoSubscriptionId and userId as composite keys instead of relying solely on
dodoPaymentId, ensuring the placeholder can be properly located and updated.
In `@server/vercel.json`:
- Around line 8-10: The blanket rewrite rule with source pattern "/(.*)" and
destination "/api" routes all incoming requests through the serverless function,
preventing Vercel's CDN from directly serving static assets. Change the rewrite
source pattern to be more granular, such as "/api/(.*)" instead, so that only
API requests are routed to the serverless function while static assets at paths
like /uploads and /public can be served directly by the CDN. Alternatively,
explicitly configure Vercel routes to handle static files separately from API
requests to ensure optimal performance and proper request routing.
---
Outside diff comments:
In `@server/src/module/opensource/opensource.service.ts`:
- Around line 433-450: The opensourceRepo.create and repoRequest.update
operations are performed sequentially without transactional guarantees, which
can cause data inconsistency if the first operation succeeds but the second
fails. Wrap both the prisma.opensourceRepo.create call and the subsequent
prisma.repoRequest.update call within a prisma.$transaction block to ensure both
operations are atomic, preventing the scenario where a repo is created but the
request status is not updated, which would lead to duplicate repos if an admin
retries the approval.
---
Nitpick comments:
In `@client/src/components/common/ScrollToTop.tsx`:
- Around line 12-15: In the ScrollToTop component, remove state from the
useEffect dependency array and keep only pathname. The effect currently re-runs
whenever state changes due to state being listed as a dependency, but the
intended behavior is to scroll to top only when pathname changes. Since the
state variable is already accessible within the effect's scope, the condition
checking for scrollToTop will still work correctly with just [pathname] as the
dependency.
In `@client/src/components/DynamicFieldRenderer.tsx`:
- Around line 28-72: The FileUploadField component maintains local fileError
state that only clears when the user selects a new file, but if the parent
component externally clears the value prop (such as during a form reset), the
error message persists without a selected file, creating poor UX. Add a
useEffect hook that monitors the value prop and automatically clears fileError
by calling setFileError(null) whenever value becomes empty or falsy, ensuring
the local error state stays synchronized with the parent value prop. Include
value as a dependency in the useEffect dependency array.
In `@client/src/components/ProtectedRoute.tsx`:
- Around line 7-12: The useEffect hook in the RedirectWithToast function reads
the message prop in the toast.error call but omits it from the dependency array.
Add message to the dependency array of the useEffect (changing [] to [message])
to comply with React Hooks rules and avoid ESLint warnings about missing
dependencies.
In `@client/src/module/student/opensource/components/GuideSectionPage.tsx`:
- Around line 88-95: The dismissShortcutHint function has an empty catch block
when calling localStorage.setItem that swallows errors silently and violates
lint rules. Replace the empty catch block with either a no-op comment explaining
the intent to gracefully ignore storage errors, or add a lightweight fallback
such as a console warning or error log to signal the failure while still
allowing the hint dismissal to proceed. Ensure the catch handler makes the
intent explicit rather than leaving it empty.
In `@server/src/__tests__/learn.service.test.ts`:
- Line 104: Extract the fallback readiness calculation formula (completedCount *
10 + dsaCount * 5 with Math.min and Math.max bounds) from the test into a
separate pure function called calculateFallbackReadiness in the service file.
Update the service implementation to use this extracted function, then modify
the test expectation in the describe block to call calculateFallbackReadiness(2,
5) instead of hardcoding the formula calculation. Finally, create a dedicated
test file to thoroughly test the calculateFallbackReadiness function
independently to verify the formula logic and edge cases.
In `@server/src/__tests__/leetcode.service.test.ts`:
- Around line 129-168: The test description for the test case starting with
"should update existing records that are NOT solved, and keep their original
solve dates" is misleading because the actual assertion on line 164 verifies
that solvedAt is updated to the LeetCode timestamp (new Date(1672531200 *
1000)), not the original date. Update the test description string in the it()
function to accurately reflect the actual behavior being tested, such as
clarifying that the solve dates are set from LeetCode submission timestamps
rather than being preserved.
In `@server/src/__tests__/opensource.service.test.ts`:
- Around line 70-91: The makeRequest helper function does not include a user
field, but the service's approveRepoRequest method attempts to access
request.user.email and request.user.name when sending emails. Add a user field
to the object returned by makeRequest with name and email properties to match
the data structure returned by the service's findUnique query with the user
select clause. Also update the mock on lines 164-177 to include the same user
field structure so the email sending flow is properly validated.
In `@server/src/cron/daily-cron.route.ts`:
- Around line 148-151: The authorization header comparison in the route handler
is using a standard string comparison with `!==` which is vulnerable to timing
attacks. Replace this vulnerable comparison by importing crypto.timingSafeEqual
from the Node.js crypto module and using it to perform a constant-time
comparison between the Bearer token extracted from req.headers.authorization and
the secret value. Ensure both values are converted to Buffers before passing
them to timingSafeEqual for proper timing-safe comparison.
In `@server/src/middleware/error.middleware.ts`:
- Around line 157-170: The fallback substring matching logic using
msg.includes() checks for patterns like "not found", "already
exists/registered/applied", and "unauthorized" can produce false positives when
these substrings appear in unexpected contexts within error messages. To fix
this, replace the simple substring matching with more context-aware pattern
matching using regular expressions with word boundaries (like /\bnot found\b/i
for the "not found" check) to ensure the target words match as whole words
rather than arbitrary substrings, thereby reducing the risk of misclassifying
error messages that happen to contain these terms in unintended contexts.
In `@server/src/module/auth/auth.routes.ts`:
- Line 69: The logout endpoint now requires authMiddleware, which means only
authenticated users can logout. Verify that the authController.logout() method's
session invalidation logic actually requires user identification. Check if the
logout implementation (the logout method in authController) uses the
authenticated user information from the request to invalidate tokens or
sessions. If the logout logic does require user context, the authMiddleware
requirement is correct; if not, consider removing the authMiddleware parameter
from the authRouter.post() route definition to allow unauthenticated logout
requests.
In `@server/src/module/learn/learn.service.ts`:
- Line 161: Replace the console.error call in the Gemini exception handling
block with the structured logger utility from server/src/utils/logger.ts. Import
the logger at the top of the learn.service.ts file and replace the console.error
statement that logs "Gemini exception in readiness report, using fallback:" with
the appropriate logger method to maintain consistent, structured error logging
across the codebase.
In `@server/vercel.json`:
- Around line 3-7: The maxDuration setting of 60 seconds in the "api/index.ts"
configuration may be insufficient for heavy jobs involving scraping and AI
processing, but this limitation is not documented. Add a comment or
documentation in vercel.json explaining that the 60-second limit applies to both
light and heavy cron jobs, that heavy jobs may be skipped on the Hobby tier due
to the 55-second soft budget, and that upgrading to Pro tier (which supports
~300 seconds) is recommended for heavier workloads. This will ensure future
maintainers understand the tier-specific constraints and the trade-off that
skipped jobs only retry the next day.
🪄 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 Plus
Run ID: 3234be64-5b70-4bee-a8c8-a28fc9f6f63a
⛔ Files ignored due to path filters (1)
client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (69)
.gitignoreclient/.env.exampleclient/package.jsonclient/src/App.tsxclient/src/components/DynamicFieldRenderer.tsxclient/src/components/Navbar.tsxclient/src/components/ProtectedRoute.tsxclient/src/components/common/ScrollToTop.tsxclient/src/lib/auth.store.tsclient/src/lib/axios.tsclient/src/lib/types/user.types.tsclient/src/module/recruiter/applications/ApplicationsList.tsxclient/src/module/student/companies/CompanyDetailPage.tsxclient/src/module/student/jobs/JobBrowsePage.tsxclient/src/module/student/opensource/AmbassadorPage.tsxclient/src/module/student/opensource/CertificateViewPage.tsxclient/src/module/student/opensource/GSoCReposPage.tsxclient/src/module/student/opensource/MySubmissionsPage.tsxclient/src/module/student/opensource/ProgramTrackerPage.tsxclient/src/module/student/opensource/RepoCard.tsxclient/src/module/student/opensource/components/GuideSectionPage.tsxclient/src/module/student/opensource/learning-paths.context.test.tsxclient/src/module/student/sql/SqlExercisePage.tsxserver/api/index.tsserver/package.jsonserver/src/__tests__/learn.service.test.tsserver/src/__tests__/leetcode.service.test.tsserver/src/__tests__/opensource.service.test.tsserver/src/cron/ambassador-eligibility.cron.tsserver/src/cron/daily-cron.route.tsserver/src/cron/deadline-alerts.cron.tsserver/src/cron/internhack-ai.cron.tsserver/src/cron/job-cleanup.cron.tsserver/src/cron/scheduled-email-worker.tsserver/src/cron/scheduled-emails.tsserver/src/cron/signals-cleanup.tsserver/src/cron/subscription-expiry.tsserver/src/database/db.tsserver/src/database/prisma/migrations/20260618000000_add_repo_id_to_repo_request/migration.sqlserver/src/database/prisma/schema/base.prismaserver/src/index.tsserver/src/lib/providers/groq.provider.tsserver/src/lib/providers/openrouter.provider.tsserver/src/middleware/error.middleware.tsserver/src/middleware/rate-limit.middleware.tsserver/src/module/ambassador/ambassador.routes.tsserver/src/module/ambassador/ambassador.service.tsserver/src/module/ats/ats.validation.tsserver/src/module/auth/auth.routes.tsserver/src/module/auth/auth.validation.tsserver/src/module/dsa/leetcode.service.tsserver/src/module/job-agent/job-agent.routes.tsserver/src/module/learn/learn.controller.tsserver/src/module/learn/learn.routes.tsserver/src/module/learn/learn.service.tsserver/src/module/learn/learn.validation.tsserver/src/module/opensource/opensource.routes.tsserver/src/module/opensource/opensource.service.tsserver/src/module/payment/payment.service.tsserver/src/module/payment/payment.validation.tsserver/src/module/roadmap/roadmap.controller.tsserver/src/module/roadmap/roadmap.service.tsserver/src/module/scraper/scraper.routes.tsserver/src/module/scraper/scraper.service.tsserver/src/module/signals/signals.service.tsserver/src/module/upload/upload.routes.tsserver/src/utils/s3.utils.tsserver/tsconfig.api.jsonserver/vercel.json
| <Link | ||
| to={href} | ||
| onClick={onClick} | ||
| role="menuitem" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if MobileNavLink is properly nested in a menu container with role="menu"
# Search for MobileNavLink usage and surrounding menu/nav context
rg -n -B10 -A2 'MobileNavLink' client/src/components/Navbar.tsx | head -60Repository: saurabhhhcodes/InternHack
Length of output: 1216
🏁 Script executed:
sed -n '408,430p' client/src/components/Navbar.tsxRepository: saurabhhhcodes/InternHack
Length of output: 544
🏁 Script executed:
sed -n '340,360p' client/src/components/Navbar.tsxRepository: saurabhhhcodes/InternHack
Length of output: 940
🏁 Script executed:
rg -n "onKey|keydown|keyup|KeyBoard" client/src/components/Navbar.tsxRepository: saurabhhhcodes/InternHack
Length of output: 51
Implement keyboard interactions for the ARIA menu pattern or remove menu roles.
The role="menuitem" on line 421 requires the full ARIA menu pattern to be properly accessible, including arrow key navigation between items, Enter to activate, and Escape to close the menu. Currently, no keyboard event handlers are implemented. Either:
- Implement full keyboard support (arrow keys, Enter, Escape), or
- Remove the
role="menu"androle="menuitem"attributes and rely on implicit link semantics instead.
For a mobile navigation pattern where menu items are just links, the implicit link role is often more appropriate.
🤖 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 `@client/src/components/Navbar.tsx` at line 421, The Navbar component at line
421 uses role="menuitem" which requires complete keyboard support for ARIA menu
patterns including arrow key navigation, Enter to activate, and Escape to close.
Since the menu items are functioning as links in a mobile navigation pattern,
remove the role="menuitem" attribute from the menu item element and remove any
parent role="menu" attributes if present. This allows the components to use
implicit link semantics which is more appropriate for this use case and does not
require additional keyboard event handlers.
Comment on lines
31
to
33
| if (user && !user.isVerified && user.role !== "ADMIN") { | ||
| return <Navigate to={`/verify-email?email=${encodeURIComponent(user.email)}`} replace />; | ||
| return <Navigate to="/verify-email" replace state={{ email: user.email }} />; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify VerifyEmailPage reads email from location.state
rg -n -A 5 -B 2 'useLocation.*state.*email|location\.state\.email' --type=ts --type=tsx -g 'VerifyEmailPage*'Repository: saurabhhhcodes/InternHack
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Find VerifyEmailPage component file
fd -i 'verifyemail' --type f
# Also search for useLocation and state.email usage
rg -n 'useLocation' --type ts -i 'verify'Repository: saurabhhhcodes/InternHack
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Search for location.state.email or useLocation pattern more broadly
rg -n -A 5 'useLocation\(\)' | grep -A 5 -B 5 -i 'verifyemail'
# Also check direct references to location.state.email
rg -n 'location\.state' -g '*Verify*'Repository: saurabhhhcodes/InternHack
Length of output: 51
🏁 Script executed:
cat client/src/module/auth/VerifyEmailPage.tsxRepository: saurabhhhcodes/InternHack
Length of output: 7398
Fix email retrieval in VerifyEmailPage to use location.state instead of query parameters.
VerifyEmailPage currently reads email from useSearchParams() (query parameters), but ProtectedRoute passes it via location.state. This mismatch means the email won't be passed correctly from ProtectedRoute. Update VerifyEmailPage to import and use useLocation() to read location.state.email, ensuring the email is properly received via React Router state as intended.
🤖 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 `@client/src/components/ProtectedRoute.tsx` around lines 31 - 33,
VerifyEmailPage is currently reading the email from query parameters using
useSearchParams(), but ProtectedRoute is passing it via location.state. In
VerifyEmailPage, replace the useSearchParams() hook import and usage with
useLocation(), then access the email value from location.state.email instead of
from the search parameters. This will ensure the email is properly received from
the Navigate state passed by ProtectedRoute.
| // On EC2/local this ran inside the app.listen() callback. On Vercel there is no | ||
| // listen callback, so the one-time provider init must run here when the function | ||
| // module is first loaded. Failures are swallowed so a cold start still serves. | ||
| await initServiceProviders().catch(() => {}); |
There was a problem hiding this comment.
Log initialization errors instead of swallowing them silently.
The catch(() => {}) swallows all errors from initServiceProviders(), which could hide critical initialization failures (database connection issues, invalid configs, etc.). While allowing the cold start to proceed makes sense, operators need visibility into why AI features might fail at runtime.
🔍 Proposed fix to log errors while still allowing startup
-await initServiceProviders().catch(() => {});
+await initServiceProviders().catch((err) => {
+ console.error("[Vercel] AI provider initialization failed on cold start:", err);
+});📝 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.
Suggested change
| await initServiceProviders().catch(() => {}); | |
| await initServiceProviders().catch((err) => { | |
| console.error("[Vercel] AI provider initialization failed on cold start:", err); | |
| }); |
🤖 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/api/index.ts` at line 7, The catch block in the initServiceProviders()
call is silently swallowing all errors without any logging, which hides critical
initialization failures from operators. Replace the empty catch handler with a
catch block that logs the error details (using an appropriate logger) so
visibility is maintained into why AI features might fail at runtime, while still
allowing the application startup to continue.
| let cronJob: cron.ScheduledTask | null = null; | ||
|
|
||
| // How long to keep scraped/indexed jobs. Override with JOB_CLEANUP_RETAIN_DAYS. | ||
| const RETAIN_DAYS = Number(process.env["JOB_CLEANUP_RETAIN_DAYS"] ?? 30); |
There was a problem hiding this comment.
Validate RETAIN_DAYS to prevent invalid date calculations.
If JOB_CLEANUP_RETAIN_DAYS is set to a non-numeric value, Number() returns NaN, which would cause the cutoff date calculation on line 22 to produce an invalid date. This could lead to unexpected behavior in the deletion queries.
🛡️ Proposed fix to add validation
-const RETAIN_DAYS = Number(process.env["JOB_CLEANUP_RETAIN_DAYS"] ?? 30);
+const envRetainDays = Number(process.env["JOB_CLEANUP_RETAIN_DAYS"] ?? 30);
+const RETAIN_DAYS = Number.isFinite(envRetainDays) && envRetainDays > 0 ? envRetainDays : 30;📝 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.
Suggested change
| const RETAIN_DAYS = Number(process.env["JOB_CLEANUP_RETAIN_DAYS"] ?? 30); | |
| const envRetainDays = Number(process.env["JOB_CLEANUP_RETAIN_DAYS"] ?? 30); | |
| const RETAIN_DAYS = Number.isFinite(envRetainDays) && envRetainDays > 0 ? envRetainDays : 30; |
🤖 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/cron/job-cleanup.cron.ts` at line 8, The RETAIN_DAYS constant
assignment uses Number() to convert the environment variable but does not
validate that the result is a valid number. If JOB_CLEANUP_RETAIN_DAYS is set to
a non-numeric value, Number() will return NaN, which will break the date
calculation that uses RETAIN_DAYS. Add validation after the constant assignment
to check if RETAIN_DAYS is a valid number using Number.isNaN() or isNaN(), and
if it's not valid, either log a warning and use the default value of 30, or
throw an error to prevent invalid date calculations downstream.
Comment on lines
+5
to
+19
| function isAllowedUrl(url: string): boolean { | ||
| if (url.startsWith("/uploads/")) return true; | ||
| if (!S3_BUCKET) return url.startsWith("https://"); | ||
| try { | ||
| const parsed = new URL(url); | ||
| if (parsed.protocol !== "https:") return false; | ||
| return ( | ||
| parsed.hostname === `${S3_BUCKET}.s3.amazonaws.com` || | ||
| (parsed.hostname.startsWith(`${S3_BUCKET}.s3.`) && | ||
| parsed.hostname.endsWith(".amazonaws.com")) | ||
| ); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Security risk: Overly permissive validation when S3_BUCKET is not configured.
Line 7 allows any https:// URL when AWS_S3_BUCKET is empty or unset. This permits resume URLs from arbitrary external domains in non-production environments, which could enable SSRF attacks or allow processing of malicious/untrusted resume files.
Consider failing validation when S3_BUCKET is not configured, or at minimum log a warning that production-grade validation is disabled.
🔒 Recommended fix
function isAllowedUrl(url: string): boolean {
if (url.startsWith("/uploads/")) return true;
- if (!S3_BUCKET) return url.startsWith("https://");
+ if (!S3_BUCKET) {
+ // In production, S3_BUCKET must be configured for security
+ console.warn("[Security] AWS_S3_BUCKET not configured - resume URL validation disabled");
+ return url.startsWith("https://");
+ }
try {Or, for stricter security:
function isAllowedUrl(url: string): boolean {
if (url.startsWith("/uploads/")) return true;
- if (!S3_BUCKET) return url.startsWith("https://");
+ if (!S3_BUCKET) {
+ throw new Error("AWS_S3_BUCKET must be configured for resume URL validation");
+ }
try {📝 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.
Suggested change
| function isAllowedUrl(url: string): boolean { | |
| if (url.startsWith("/uploads/")) return true; | |
| if (!S3_BUCKET) return url.startsWith("https://"); | |
| try { | |
| const parsed = new URL(url); | |
| if (parsed.protocol !== "https:") return false; | |
| return ( | |
| parsed.hostname === `${S3_BUCKET}.s3.amazonaws.com` || | |
| (parsed.hostname.startsWith(`${S3_BUCKET}.s3.`) && | |
| parsed.hostname.endsWith(".amazonaws.com")) | |
| ); | |
| } catch { | |
| return false; | |
| } | |
| } | |
| function isAllowedUrl(url: string): boolean { | |
| if (url.startsWith("/uploads/")) return true; | |
| if (!S3_BUCKET) { | |
| // In production, S3_BUCKET must be configured for security | |
| console.warn("[Security] AWS_S3_BUCKET not configured - resume URL validation disabled"); | |
| return url.startsWith("https://"); | |
| } | |
| try { | |
| const parsed = new URL(url); | |
| if (parsed.protocol !== "https:") return false; | |
| return ( | |
| parsed.hostname === `${S3_BUCKET}.s3.amazonaws.com` || | |
| (parsed.hostname.startsWith(`${S3_BUCKET}.s3.`) && | |
| parsed.hostname.endsWith(".amazonaws.com")) | |
| ); | |
| } catch { | |
| return false; | |
| } | |
| } |
🤖 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/ats/ats.validation.ts` around lines 5 - 19, The
isAllowedUrl function has a security vulnerability where it allows any https URL
when S3_BUCKET is not configured. The condition that checks if S3_BUCKET is
falsy and then returns true for any https URL is overly permissive and could
enable SSRF attacks or processing of untrusted resume files. Replace this
permissive check with a stricter validation that either rejects the URL entirely
(returns false) when S3_BUCKET is not configured, or at minimum logs a warning
indicating that production-grade validation is disabled before proceeding.
Comment on lines
+103
to
+142
| const systemPrompt = ` | ||
| You are an expert tech recruiter and coding coach assessing student readiness indicators. | ||
|
|
||
| Target Role: ${targetRole} | ||
| Hiring Company Tier: ${companyTier} | ||
| Prep Time: ${availableTime} | ||
|
|
||
| Student Learning Progress: | ||
| - Completed Interview Lessons Count: ${completedLessonsCount} (Lesson IDs: ${completedLessonIds.join(", ") || "None"}) | ||
| - Solved DSA Problems Count: ${totalDsaSolved} | ||
| - Solved SQL Exercises Count: ${totalSqlSolved} | ||
| - Solved Aptitude Questions Count: ${totalAptitudeSolved} | ||
|
|
||
| Analyze this student's readiness based on their progress and target profile. | ||
| - Calculate a realistic overallReadiness score (0 to 100). Consider their role and tier (e.g. FAANG requires higher problem counts and system design knowledge). | ||
| - Estimate timeline to ready (e.g., "3 weeks", "2 months"). | ||
| - Provide today's study priority based on what's missing or what's next. | ||
| - List 2-3 strongAreas with scores (0-100). | ||
| - List 2-3 gapAreas with scores (0-100). | ||
| - Suggest a relevant mockInterviewQuestion (containing title and description) tailored to their target role. | ||
|
|
||
| Respond strictly with a single JSON object. Do not include markdown ticks: | ||
| { | ||
| "overallReadiness": 65, | ||
| "estimatedTimeToReady": "3 weeks", | ||
| "todaysPriority": "Solve 2 more Binary Tree problems + complete Node.js authentication lesson", | ||
| "strongAreas": [ | ||
| { "topic": "HTML/CSS", "score": 90 }, | ||
| { "topic": "React Hooks", "score": 80 } | ||
| ], | ||
| "gapAreas": [ | ||
| { "topic": "System Design", "score": 20 }, | ||
| { "topic": "TypeScript", "score": 45 } | ||
| ], | ||
| "mockInterviewQuestion": { | ||
| "title": "Implement a Custom useFetch Hook", | ||
| "description": "Create a hook handling network execution state loops cleanly." | ||
| } | ||
| } | ||
| `; |
There was a problem hiding this comment.
Sanitize user input before embedding in the AI prompt.
The system prompt directly interpolates user-controlled fields (targetRole, companyTier, availableTime) without sanitization. While Gemini's structured output reduces exploit risk, malicious inputs could still manipulate the AI's reasoning or cause unexpected prompt behavior. Consider validating these fields against allowed enumerations or patterns before embedding them in the prompt.
🔒 Recommended sanitization approach
Add input validation in the validation schema or service:
async calculateReadinessReport(data: {
userId: number | string;
targetRole: string;
companyTier: string;
availableTime: string;
}) {
const { targetRole, companyTier, availableTime } = data;
+
+ // Sanitize inputs to prevent prompt injection
+ const sanitizedRole = targetRole.replace(/[^\w\s-]/g, '').slice(0, 100);
+ const sanitizedTier = companyTier.replace(/[^\w\s-]/g, '').slice(0, 50);
+ const sanitizedTime = availableTime.replace(/[^\w\s-]/g, '').slice(0, 50);
const userIdNum = typeof data.userId === "string" ? parseInt(data.userId, 10) : data.userId;Or add enum constraints in the validation schema for more robust protection.
🤖 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/learn/learn.service.ts` around lines 103 - 142, The
user-controlled variables targetRole, companyTier, and availableTime are
directly interpolated into the systemPrompt template literal without validation.
Before constructing the system prompt, validate these three fields against their
allowed enumeration values or permitted patterns. Add validation logic in the
service method to ensure only safe, expected values are accepted, and reject or
sanitize any unexpected inputs. This prevents malicious users from injecting
arbitrary content into the AI prompt that could manipulate the model's reasoning
or cause unexpected behavior.
| @@ -1,4 +1,4 @@ | |||
| import DodoPayments from "dodopayments"; | |||
| import { DodoPayments } from "dodopayments"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual export from dodopayments package
fd -t f "package.json" --exec grep -l "dodopayments" {} \; | head -1 | xargs dirname | xargs -I {} sh -c 'cat {}/node_modules/dodopayments/package.json 2>/dev/null | jq -r ".main, .exports"'
# Check if other files in the codebase use default or named import
rg -n "from ['\"]dodopayments['\"]" --type tsRepository: saurabhhhcodes/InternHack
Length of output: 161
🌐 Web query:
dodopayments npm package v2.32.0 export pattern named import default import
💡 Result:
The dodopayments npm package (including version 2.32.0) is designed primarily for a default import pattern [1][2]. The standard way to import and initialize the library in both JavaScript and TypeScript is as follows: import DodoPayments from 'dodopayments'; const client = new DodoPayments({ bearerToken: process.env['DODO_PAYMENTS_API_KEY'], environment: 'live_mode', }); In TypeScript, while the client itself is exported as a default export, the library also provides access to various request parameter and response types via the DodoPayments namespace [2]. You can reference these types directly from the imported class object: import DodoPayments from 'dodopayments'; const params: DodoPayments.CheckoutSessionCreateParams = { product_cart: [{ product_id: 'product_id', quantity: 0 }], }; This pattern allows you to maintain clean code by using the default export for the client instance while still leveraging the package's comprehensive TypeScript type definitions [2].
Citations:
- 1: https://github.com/dodopayments/dodopayments-typescript
- 2: https://github.com/dodopayments/dodopayments-typescript/
Use default import for dodopayments v2.32.0.
The named import { DodoPayments } is incorrect. The dodopayments package exports a default export and should be imported as:
import DodoPayments from "dodopayments";🤖 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/payment/payment.service.ts` at line 1, The import statement
for the DodoPayments module is using incorrect named import syntax with curly
braces. Change the import statement at the top of the payment.service.ts file
from a named import to a default import by removing the curly braces around
DodoPayments, since the dodopayments v2.32.0 package exports a default export
rather than a named export.
Comment on lines
+226
to
+239
| if (!payment) { | ||
| // No payment record exists yet — create a placeholder that will be | ||
| // updated to SUCCESS when payment.succeeded arrives | ||
| await tx.payment.create({ | ||
| data: { | ||
| userId, | ||
| amount: 0, | ||
| currency: "USD", | ||
| status: "PENDING", | ||
| dodoSubscriptionId: sub.subscription_id, | ||
| plan, | ||
| billing: sub.metadata["billing"] ?? "monthly", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Dodo Payments subscription.active webhook include checkout_session_id in the payload or metadata?
💡 Result:
Dodo Payments’ subscription.active webhook payload does not document any field named checkout_session_id, nor does it document returning it in either the payload or a dedicated metadata section. In Dodo’s webhook model, the top-level event is { business_id, type, timestamp, data }, where the event-specific content is under data; the subscription webhook documentation points you to the subscription object fields within data, rather than any checkout-session identifier [1][2]. The subscription.active integration examples also show handling the subscription object from event.data, not checkout session identifiers [3]. If you need checkout_session_id, the supported approach in Dodo’s docs is to persist your own linkage (e.g., via custom fields/custom_field_responses during the checkout/session flow) and read it from the relevant webhook payload rather than relying on checkout_session_id being included for subscription.active [4].
Citations:
- 1: https://docs.dodopayments.com/developer-resources/webhooks
- 2: https://docs.dodopayments.com/developer-resources/webhooks/intents/subscription
- 3: https://docs.dodopayments.com/developer-resources/subscription-integration-guide
- 4: https://docs.dodopayments.com/developer-resources/checkout-session
Placeholder payment will remain orphaned as PENDING.
The placeholder created here has no dodoPaymentId. When payment.succeeded arrives later, the handler at lines 104-112 uses where: { dodoPaymentId: payment.checkout_session_id } to update the record. Since the placeholder's dodoPaymentId is null, it won't match—leaving this payment permanently in PENDING status.
The subscription.active webhook does not include checkout_session_id, making Option 1 (metadata-based linking) insufficient. Dodo's documented approach requires using custom fields to persist data through the checkout flow. Choose one of these approaches instead:
-
Reconciliation job – Periodically match orphaned PENDING payments to subscriptions by
dodoSubscriptionIdanduserId, then update with payment details oncepayment.succeededarrives. -
Accept and document – Add a comment explaining that placeholder records remain PENDING until actual payment webhook arrives; link them by subscription ID in queries rather than payment ID.
Custom fields approach (if Dodo's custom fields are available for your integration)
If your Dodo integration supports custom fields that flow through checkout → subscription webhooks, you could pass the checkout session ID via custom_field_responses during checkout, then retrieve it from the subscription.active webhook to set dodoPaymentId. However, this requires verifying Dodo's custom fields implementation for your setup (see Dodo's custom fields documentation).
🤖 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/payment/payment.service.ts` around lines 226 - 239, The
placeholder payment created in tx.payment.create() lacks a dodoPaymentId, but
the payment update handler at lines 104-112 searches for records using where: {
dodoPaymentId: payment.checkout_session_id }. Since the placeholder's
dodoPaymentId is null, it will never be matched and will remain orphaned in
PENDING status. Implement a reconciliation approach by either creating a
periodic job that matches orphaned PENDING payments by dodoSubscriptionId and
userId together, then updates them when payment.succeeded arrives, or modify the
payment update logic in the handler to match using dodoSubscriptionId and userId
as composite keys instead of relying solely on dodoPaymentId, ensuring the
placeholder can be properly located and updated.
Comment on lines
+8
to
+10
| "rewrites": [ | ||
| { "source": "/(.*)", "destination": "/api" } | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for static asset serving configuration
rg -n "express\.static|app\.use.*static" server/src/
# Check for root-level routes that might be affected
rg -n "app\.get\(\s*['\"]\/['\"]" server/src/
# Check for public directory or static file references
fd -t d "public|static" server/Repository: saurabhhhcodes/InternHack
Length of output: 447
🏁 Script executed:
# Check the structure around the static file serving lines
sed -n '200,220p' server/src/index.tsRepository: saurabhhhcodes/InternHack
Length of output: 974
🏁 Script executed:
# Check the structure around line 336
sed -n '330,345p' server/src/index.tsRepository: saurabhhhcodes/InternHack
Length of output: 666
🏁 Script executed:
# Look for API setup and how it handles requests
rg -n "app\.use.*api|/api" server/src/index.ts | head -20Repository: saurabhhhcodes/InternHack
Length of output: 1239
🏁 Script executed:
# Check the overall middleware order to understand request flow
head -n 400 server/src/index.ts | grep -n "app\.use\|app\.get\|app\.post" | tail -30Repository: saurabhhhcodes/InternHack
Length of output: 1729
🏁 Script executed:
# Check for catch-all routes or error handlers
rg -n "app\.use\(\)" server/src/index.tsRepository: saurabhhhcodes/InternHack
Length of output: 51
🏁 Script executed:
# Check what comes after the static middleware - look for error handling
sed -n '336,380p' server/src/index.tsRepository: saurabhhhcodes/InternHack
Length of output: 1658
🏁 Script executed:
# Check the full vercel.json configuration
cat server/vercel.jsonRepository: saurabhhhcodes/InternHack
Length of output: 411
🏁 Script executed:
# Check if there's any special handling for static files in index.ts
rg -n "404|catch.all|wildcard|fallback" server/src/index.tsRepository: saurabhhhcodes/InternHack
Length of output: 51
🏁 Script executed:
# Check the api/index.ts file to understand how the Express app is exported
cat server/api/index.tsRepository: saurabhhhcodes/InternHack
Length of output: 548
🏁 Script executed:
# Check if there are any files explaining the static file serving setup
find server -name "*.md" -o -name "*.txt" | xargs grep -l "static\|vercel\|rewrite" 2>/dev/nullRepository: saurabhhhcodes/InternHack
Length of output: 53
🏁 Script executed:
# Look for how the public directory is structured
fd -t f "." server/public/ 2>/dev/null | head -20Repository: saurabhhhcodes/InternHack
Length of output: 51
The app does serve static assets, but the blanket rewrite is architecturally questionable.
The Express app is configured with static middleware at lines 209 (/uploads) and 336 (/public), so requests for static files will be served. However, the rewrite rule routes ALL requests through the /api serverless function rather than letting Vercel's CDN serve static assets directly. This means:
- Static file requests go through the Express middleware stack and serverless cold starts instead of direct CDN delivery
- If Vercel's rewrite doesn't preserve the original request path, static middleware won't match and requests will fail
Confirm that static assets are accessible in practice and consider using a more granular rewrite (e.g., source: "/api/(.*)") with explicit static file handling through Vercel's built-in routes if performance is a concern.
🤖 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/vercel.json` around lines 8 - 10, The blanket rewrite rule with source
pattern "/(.*)" and destination "/api" routes all incoming requests through the
serverless function, preventing Vercel's CDN from directly serving static
assets. Change the rewrite source pattern to be more granular, such as
"/api/(.*)" instead, so that only API requests are routed to the serverless
function while static assets at paths like /uploads and /public can be served
directly by the CDN. Alternatively, explicitly configure Vercel routes to handle
static files separately from API requests to ensure optimal performance and
proper request routing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
15 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: Sachinchaurasiya360#2356 and Sachinchaurasiya360#2257
Changes
stateinstead of URL query param to avoid leaking PII in browser history/URL bar.Before
/verify-email?email=user@example.com— email visible in URLAfter
<Navigate to="/verify-email" state={{ email: user.email }} />— email stays in router state only<RedirectWithToast ... message="You do not have permission to access this page" />— user sees error toast before redirectSummary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes