fix: incorrect Error constructor syntax in files #2824
Conversation
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
Contributor
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughUpdated error handling at two frontend locations: Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-07-12T17:14:28.536ZApplied to files:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
34-36: Error constructor syntax is now correct.Same fix and recommendations as the
checkIfProjectLeaderfunction above. Consider applying consistent error handling improvements to both functions.
🧹 Nitpick comments (1)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
20-22: Error constructor syntax is now correct.The fix properly consolidates the error message into a single string parameter. However, consider these improvements:
- Use error cause chaining (ES2022) to preserve the original error stack:
throw new Error('Failed to fetch project leader status', { cause: err })
- If
erris an Error object, extract its message:throw new Error(`Failed to fetch project leader status: ${err instanceof Error ? err.message : String(err)}`)
- Since this function is called during JWT callback (line 78) without visible error handling, consider whether re-throwing is appropriate or if the auth flow should degrade gracefully.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/api/auth/[...nextauth]/route.ts(2 hunks)frontend/src/components/ModuleForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ModuleForm.tsx
nileshpahari
force-pushed
the
fix/incorrect-error-constructor-usage-2803
branch
from
cf43342 to
7bcb563
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ModuleForm.tsx (1)
286-290: Good: usesetErrorinstead of throwing; consider a better fallback for non-Error valuesSwitching from throwing to
setErrorfixes the unhandled rejection and correctly surfaces failures in the UI while still clearing results and hiding suggestions. One small improvement: for non-Errorvalues, the message currently ends with an empty suffix; you could preserve more context or at least show a generic fallback, e.g.:setError( `Error fetching suggestions: ${ err instanceof Error ? err.message : String(err || 'Unknown error') }` )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/api/auth/[...nextauth]/route.ts(2 hunks)frontend/src/components/ModuleForm.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/api/auth/[...nextauth]/route.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ModuleForm.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ModuleForm.tsx (1)
289-295: Error handling is solid; consider simplifying nested ternary for readabilityThe new error handling correctly derives a user-facing message from different
errtypes and surfaces it viasetError, which is a good improvement over throwing. The nested ternary, though, is a bit hard to scan (and was flagged by Sonar); anif / else if / elsechain would be clearer.You could refactor like this:
- const errorMessage = - err instanceof Error - ? err.message - : typeof err === 'string' - ? err - : 'An unexpected error occurred' - setError(`Error fetching suggestions: ${errorMessage}`) + let errorMessage: string + + if (err instanceof Error) { + errorMessage = err.message + } else if (typeof err === 'string') { + errorMessage = err + } else { + errorMessage = 'An unexpected error occurred' + } + + setError(`Error fetching suggestions: ${errorMessage}`)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ModuleForm.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/api/auth/[...nextauth]/route.ts:13-25
Timestamp: 2025-08-10T11:08:47.258Z
Learning: In the OWASP Nest codebase (frontend/src/app/api/auth/[...nextauth]/route.ts), input validation and string trimming for authentication-related queries like `isProjectLeader` and `isMentor` are handled in the backend rather than the frontend. The backend is responsible for sanitizing and validating input parameters.
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
📚 Learning: 2025-07-12T17:14:28.536Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.536Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Applied to files:
frontend/src/components/ModuleForm.tsx
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/ModuleForm.tsx
[warning] 292-294: Extract this nested ternary operation into an independent statement.
nileshpahari
force-pushed
the
fix/incorrect-error-constructor-usage-2803
branch
from
1fade10 to
bed93a6
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Proposed change
Correct the syntax of the
Errorconstructor, in order to preserve the original error.Files modified
frontend/src/app/api/auth/[...nextauth]/route.tsfrontend/src/components/ModuleForm.tsxResolves #2803
Checklist
make check-testlocally; all checks and tests passed.