-
Notifications
You must be signed in to change notification settings - Fork 169
fix: add .catch() to promise chains #1094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,7 @@ export async function PATCH(req: Request, { params }: { params: Promise<{ id: st | |
| const email = user?.primaryEmailAddress?.emailAddress; | ||
|
|
||
| const { id } = await params; | ||
| const doubtId = parseInt(id); | ||
| const doubtId = parseInt(id, 10); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The permissive Severity Level: Major
|
||
|
|
||
| if (isNaN(doubtId)) { | ||
| return NextResponse.json({ error: "Invalid doubt ID" }, { status: 400 }); | ||
|
|
@@ -320,7 +320,7 @@ export async function DELETE(req: Request, { params }: { params: Promise<{ id: s | |
| } | ||
|
|
||
| const { id } = await params; | ||
| const doubtId = parseInt(id); | ||
| const doubtId = parseInt(id, 10); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The DELETE handler converts the route parameter to a number but never rejects Severity Level: Critical 🚨- ❌ Invalid deletion requests return server errors.
- ⚠️ Prefixed IDs can delete unintended authorized doubts.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/app/api/doubts/action/[id]/route.ts
**Line:** 323:323
**Comment:**
*Api Mismatch: The DELETE handler converts the route parameter to a number but never rejects `NaN` or other invalid values before using it in the database query. Wholly invalid IDs can therefore reach the database and produce an internal error instead of the expected 400 response, while prefixed values can target a different real doubt; apply the same strict validation used by the PATCH handler.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
|
|
||
| const [doubt] = await db.select().from(doubtsTable).where(and(eq(doubtsTable.id, doubtId), isNull(doubtsTable.deletedAt))).limit(1); | ||
| if (!doubt) return NextResponse.json({ error: "Doubt not found" }, { status: 404 }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,7 +55,7 @@ export async function GET(req: Request) { | |
| try { | ||
| const user = await currentUser(); | ||
| const email = user?.primaryEmailAddress?.emailAddress ?? null; | ||
| const classroomId = classroomIdStr ? parseInt(classroomIdStr) : null; | ||
| const classroomId = classroomIdStr ? parseInt(classroomIdStr, 10) : null; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Major
|
||
|
|
||
| if (classroomId && !email) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This subtracts calendar days in local time and then sends UTC instants to an API that compares timestamps directly. Across daylight-saving transitions, the resulting window is 23 or 25 hours per day rather than exactly the selected 7, 30, or 90-day duration, causing records near the range boundary to be included or excluded unexpectedly. Compute the range using a consistent UTC duration or explicitly define the analytics window as local calendar dates. [possible bug]
Severity Level: Major⚠️
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖