quiz_fix_cert_fix - #8
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUpdates AI model defaults; adds a Clerk-based Convex user lookup; makes grading respect course-level passing grades; broadens admin role extraction in middleware; moves certificate generation from SVG to PDF via jsPDF; adds a Convex↔Clerk role-sync API; threads coursePassingGrade through quiz components and updates UI notifications and copy. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ClerkAuth as Clerk
participant SyncRoute as NextAPI
participant ConvexAdmin as Convex
Client->>ClerkAuth: Authenticated request (POST /api/auth/sync-role)
ClerkAuth->>SyncRoute: Forward with Clerk session
SyncRoute->>ConvexAdmin: Request Convex admin token (server-side)
SyncRoute->>ConvexAdmin: Query Convex user by email
ConvexAdmin-->>SyncRoute: Convex user (role, id)
SyncRoute->>ClerkAuth: Read Clerk publicMetadata.role
alt roles differ
SyncRoute->>ClerkAuth: PATCH update publicMetadata.role
ClerkAuth-->>SyncRoute: 200 OK
SyncRoute-->>Client: 200 {previousRole, newRole, note}
else roles match
SyncRoute-->>Client: 200 {isSynced: true}
end
sequenceDiagram
autonumber
participant Client
participant WebRoute as /api/certificates/generate-pdf
participant ConvexAdmin as Convex
participant jsPDFLib as jsPDF
Client->>WebRoute: Request certificate PDF (GET/POST)
WebRoute->>ConvexAdmin: Create admin client / verify ownership (Clerk ID)
ConvexAdmin-->>WebRoute: ownership OK / user data
WebRoute->>jsPDFLib: generate PDF content (text, layout)
jsPDFLib-->>WebRoute: PDF ArrayBuffer
WebRoute-->>Client: 200 PDF (application/pdf) with Content-Disposition
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR implements several fixes related to quizzes and certificates, along with AI model configuration updates and course passing grade propagation improvements.
- Updates quiz/certificate functionality to properly respect course-level passing grades instead of storing stale values at the content item level
- Replaces SVG certificate generation with PDF generation using jsPDF library
- Improves middleware role checking to handle various Clerk metadata locations
- Updates AI model configurations (though with critical version errors)
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/types/index.ts | Added optional isCertification and passingGrade fields to chapter types, and coursePassingGrade to quiz component props |
| src/components/quiz/*.tsx | Updated quiz components to accept and use course-level passing grade as fallback |
| src/components/learnspace/*.tsx | Propagated coursePassingGrade from chapter data through quiz components |
| src/components/landing/LandingPageClient.tsx | Updated marketing copy and features list (contains typo) |
| src/components/certificates/CertificateView.tsx | Changed from SVG to PDF download functionality |
| src/app/dashboard/*/page.tsx | Replaced alert() calls with toast.success() for better UX |
| src/app/api/certificates/generate-pdf/route.ts | Complete rewrite from SVG to jsPDF-based PDF generation with improved auth handling |
| src/app/api/auth/sync-role/route.ts | New endpoint for syncing user roles between Convex and Clerk |
| middleware.ts | Enhanced role checking to handle multiple Clerk metadata locations, removed logging |
| convex/utils/grading.ts | Updated to prefer course passing grade over item-level value |
| convex/progress.ts | Re-evaluates pass/fail status using current course passing grade |
| convex/clerkAuth.ts | Added getUserByClerkId query for certificate verification |
| convex/aiConfig.ts | Updated AI model versions (contains critical errors - non-existent versions) |
| package.json | Added jspdf dependency, added unused svg2pdf.js dependency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } catch { | ||
| } catch (error) { | ||
| console.error("Certificate verification error:", error); |
There was a problem hiding this comment.
Error handling in certificate generation catches errors but only logs to console without preserving the error type or message. The generic "Certificate verification error:" message loses valuable debugging information. Consider logging the full error object or at least the error message: console.error("Certificate verification error:", error instanceof Error ? error.message : error);
| console.error("Certificate verification error:", error); | |
| console.error( | |
| "Certificate verification error:", | |
| error instanceof Error ? error.message : error | |
| ); |
| defaultModelId: "gemini-flash-latest", | ||
| }, | ||
| { | ||
| key: "capsule_generation", | ||
| name: "Capsule Generation", | ||
| description: "Generates course capsules from PDFs/Topics", | ||
| defaultModelId: "gemini-1.5-pro", | ||
| defaultModelId: "gemini-2.5-pro", | ||
| }, | ||
| { | ||
| key: "quiz_generation", | ||
| name: "Quiz Generation", | ||
| description: "Generates quizzes from notes", | ||
| defaultModelId: "gemini-1.5-flash", | ||
| defaultModelId: "gemini-2.5-pro", | ||
| }, | ||
| { | ||
| key: "notes_generation", | ||
| name: "Notes Generation", | ||
| description: "Generates interactive notes from transcripts", | ||
| defaultModelId: "gemini-1.5-pro", | ||
| defaultModelId: "gemini-2.5-pro", |
There was a problem hiding this comment.
The AI model version references in default feature configurations are inconsistent with the model definitions. "gemini-flash-latest" and "gemini-2.5-pro" do not match any of the defined model IDs in DEFAULT_MODELS. This will cause features to fail when trying to lookup non-existent models.
| "Section-by-section breakdown", | ||
| "Key terms and definitions highlighted", | ||
| "Code blocks with syntax highlighting", | ||
| "Interactive quizes", |
There was a problem hiding this comment.
Typo in feature list: "quizes" should be "quizzes". This spelling error appears in user-facing content on the landing page.
| "Interactive quizes", | |
| "Interactive quizzes", |
| if (!userId) { | ||
| // No valid session - Clerk will handle redirect | ||
| console.warn("[middleware] no userId for", pathname); | ||
| await auth.protect(); |
There was a problem hiding this comment.
The middleware removes console.warn and console.info logging statements without replacing them with a proper logging solution. This reduces observability for debugging authentication and authorization issues in production. Consider using a proper logging framework instead of completely removing logs.
| "remark-gfm": "^4.0.1", | ||
| "remark-math": "^6.0.0", | ||
| "sonner": "^2.0.6", | ||
| "svg2pdf.js": "^2.6.0", |
There was a problem hiding this comment.
The svg2pdf.js package is added to dependencies but never imported or used in the codebase. The PDF generation now uses jspdf directly instead. This unused dependency should be removed from package.json to avoid bloating the bundle.
| "svg2pdf.js": "^2.6.0", |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/app/api/certificates/generate-pdf/route.ts (1)
262-296: Missing null check for certificate query result causes potential runtime error.If
getCertificatereturnsnull(certificate not found), accessingcertificate.userIdon line 272 will throw a TypeError. The error is then caught and returns 404, which works but masks the root cause and could hide other errors.try { const certificate = await convex.query(api.certificates.getCertificate, { certificateId, }); + if (!certificate) { + return NextResponse.json( + { error: "Certificate not found" }, + { status: 404 } + ); + } + // Get the current user's Convex ID via their Clerk ID const currentUser = await convex.query(api.clerkAuth.getUserByClerkId, { clerkId: session.user.clerkId, }); const isOwnerById = currentUser && certificate.userId === currentUser._id;middleware.ts (1)
3-3: Remove unused import.The
roleFromSessionClaimsimport at line 3 is no longer used after the role extraction logic was refactored to use inline multi-location checks at lines 238-248.convex/progress.ts (1)
383-413: Re-evaluating passed items now ignores best attempt and can downgrade previously passed studentsThe new
passedGradedItemslogic re-derives pass/fail using:const coursePassingGrade = course.passingGrade ?? 70; gradingInfo.passedGradedItems = gradedProgress.filter((p) => { const percentage = p.percentage ?? ((p.bestScore ?? p.score ?? 0) / (p.maxScore ?? 100) * 100); return percentage >= coursePassingGrade; }).length;Because
p.percentageis the last attempt whilebestScoretracks the best percentage across attempts, this can now mark a student as failed (and block certificates) if they once passed but later retook the quiz and scored lower. Previously,everPassedsemantics were preserved via the storedp.passedflag andbestScore.If you intend certificates and course progress to respect a learner’s best performance, consider instead:
const coursePassingGrade = course.passingGrade ?? 70; gradingInfo.passedGradedItems = gradedProgress.filter((p) => { const bestPercentage = (p.bestScore ?? p.percentage ?? ((p.score ?? 0) / (p.maxScore ?? 100) * 100)); return bestPercentage >= coursePassingGrade; }).length;This still re-evaluates against the current course passing grade, but preserves “best attempt” behavior.
Separately,
overallGradecurrently sumsp.maxScoreandp.bestScoredirectly, even thoughbestScoreis stored as a percentage; you may want to normalize that calculation in a follow-up to avoid inflated overall grades.
🧹 Nitpick comments (6)
src/app/api/certificates/generate-pdf/route.ts (1)
210-216: Redundant HTTP method check in App Router handler.In Next.js App Router, route handlers export named functions (
POST,GET, etc.), so only POST requests reach this handler. The method check on line 211 is unnecessary.async function handler(req: NextRequest): Promise<NextResponse> { - if (req.method !== "POST") { - return NextResponse.json( - { error: "Method not allowed" }, - { status: 405 } - ); - } - // Require authentication for certificate generationmiddleware.ts (1)
234-248: Defensive multi-location role extraction looks good.The approach of probing multiple Clerk claim placements is a pragmatic solution to handle varying Clerk configurations. The chained OR logic correctly falls back through different possible locations.
Consider extracting this into a helper function to improve readability and testability:
+function extractRoleFromClaims(claims: Record<string, unknown> | null | undefined): string | undefined { + return ( + (claims?.metadata as Record<string, unknown>)?.role as string || + (claims?.publicMetadata as Record<string, unknown>)?.role as string || + (claims?.role as string) || + (claims?.public_metadata as Record<string, unknown>)?.role as string || + ((claims?.user as Record<string, unknown>)?.publicMetadata as Record<string, unknown>)?.role as string || + undefined + ); +}src/app/api/auth/sync-role/route.ts (2)
18-96: Consider adding rate limiting.This endpoint modifies Clerk user metadata (POST) and queries external services. The related
generate-pdfroute applies rate limiting (seesrc/app/api/certificates/generate-pdf/route.tslines 335-341). Consider applying similar protection here to prevent abuse.+import { withRateLimit, RATE_LIMIT_PRESETS } from "@/lib/rateLimit"; // adjust import path + export async function POST(request: NextRequest) { + const rateLimitResponse = await withRateLimit(request, RATE_LIMIT_PRESETS.DEFAULT); + if (rateLimitResponse) return rateLimitResponse; + const clerkAuthResult = await clerkAuth(); // ... }
138-145: Missing null check could cause misleadingisSyncedresult.When
convexUseris null (user not found in Convex), the response still compares roles which may produce unexpected results. Consider returning an explicit error or clearer status.+ if (!convexUser) { + return NextResponse.json({ + clerkUserId: userId, + email, + clerkPublicMetadataRole: user?.publicMetadata?.role || null, + convexRole: null, + convexUserId: null, + isSynced: false, + error: "User not found in Convex database", + }); + } + return NextResponse.json({ clerkUserId: userId, email, clerkPublicMetadataRole: user?.publicMetadata?.role || null, - convexRole: convexUser?.role || null, - convexUserId: convexUser?._id || null, - isSynced: user?.publicMetadata?.role === convexUser?.role, + convexRole: convexUser.role, + convexUserId: convexUser._id, + isSynced: user?.publicMetadata?.role === convexUser.role, });convex/utils/grading.ts (1)
41-55: Passing-grade precedence logic looks correctUsing
coursePassingGrade ?? item.passingScore ?? 70cleanly enforces the desired override behavior and matches the comments. One improvement to consider is centralizing the70default in a shared constant so grading and progress code can’t drift over time.src/lib/types/index.ts (1)
70-80: Type extensions align with new grading behaviorAdding
isCertification?/passingGrade?onChapterWithVideo.courseandcoursePassingGrade?on the quiz props matches how the rest of the code now uses course-level thresholds. To reduce future drift, you might eventually defineChapterWithVideo.coursein terms ofCourse(e.g.,Pick<Course, 'id' | 'name' | 'description' | 'isCertification' | 'passingGrade'>) instead of re-declaring fields.Also applies to: 211-228
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/images/landing/feature-certificate.pngis excluded by!**/*.png
📒 Files selected for processing (17)
convex/aiConfig.ts(2 hunks)convex/clerkAuth.ts(1 hunks)convex/progress.ts(1 hunks)convex/utils/grading.ts(1 hunks)middleware.ts(1 hunks)package.json(2 hunks)src/app/api/auth/sync-role/route.ts(1 hunks)src/app/api/certificates/generate-pdf/route.ts(7 hunks)src/app/dashboard/certificates/page.tsx(2 hunks)src/app/dashboard/profile/page.tsx(2 hunks)src/components/certificates/CertificateView.tsx(3 hunks)src/components/landing/LandingPageClient.tsx(6 hunks)src/components/learnspace/ai-tutor-panel.tsx(2 hunks)src/components/learnspace/quizzes-panel.tsx(3 hunks)src/components/quiz/QuizInterface.tsx(1 hunks)src/components/quiz/QuizResults.tsx(1 hunks)src/lib/types/index.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/components/learnspace/quizzes-panel.tsx (1)
src/lib/types/index.ts (2)
Quiz(182-185)ContentItem(40-68)
src/components/learnspace/ai-tutor-panel.tsx (1)
src/components/learnspace/quizzes-panel.tsx (1)
QuizzesPanel(33-204)
src/app/api/certificates/generate-pdf/route.ts (2)
convex/utils/certificateSignature.ts (1)
CertificateData(74-79)src/lib/convexClient.ts (1)
createConvexClient(23-39)
src/components/quiz/QuizResults.tsx (1)
src/lib/types/index.ts (1)
QuizResultsProps(220-237)
src/app/api/auth/sync-role/route.ts (2)
src/app/api/certificates/generate-pdf/route.ts (1)
POST(336-342)src/lib/convexClient.ts (1)
createConvexClient(23-39)
src/components/quiz/QuizInterface.tsx (1)
src/lib/types/index.ts (1)
QuizInterfaceProps(211-217)
🔇 Additional comments (12)
src/app/dashboard/certificates/page.tsx (1)
12-12: LGTM! Excellent UX improvement.Replacing the browser alert with a toast notification provides better user experience with a non-blocking, modern notification pattern. The implementation is clean and the message is clear.
Also applies to: 223-223
src/app/dashboard/profile/page.tsx (1)
15-15: LGTM! Consistent implementation across the codebase.The toast notification implementation matches the pattern in
certificates/page.tsx, ensuring a consistent user experience across all certificate viewing contexts.Also applies to: 347-347
src/app/api/certificates/generate-pdf/route.ts (3)
277-282: Email fallback logic provides good defense-in-depth.The email comparison fallback handles edge cases where user ID matching might fail due to data migration or sync issues. The case-insensitive comparison is appropriate.
29-200: PDF generation implementation looks solid.The jsPDF implementation correctly creates a landscape A4 certificate with proper layout, text formatting, and styling. The use of
output("arraybuffer")is appropriate for the server-side response.
336-342: Rate limiting is properly applied.Good security practice to apply rate limiting before processing certificate generation requests.
src/components/certificates/CertificateView.tsx (2)
97-141: PDF download implementation is well-structured.Good practices observed:
- Proper cleanup with
URL.revokeObjectURLafter download- State management in
finallyblock ensuresisDownloadingresets even on errors- Graceful fallback to print on failure
155-158: Download button has appropriate loading state handling.The button correctly disables during download and provides user feedback with the "Generating..." text.
src/components/learnspace/quizzes-panel.tsx (1)
33-41: Clean prop threading for course-level passing grade.The addition of the optional
coursePassingGradeprop and its propagation to child components (QuizInterfaceandQuizResults) is well-structured. This properly enables course-wide grading thresholds to flow down the component hierarchy.convex/aiConfig.ts (1)
9-31: No action needed. The model identifiersgemini-2.5-proandgpt-5.1are valid as of December 2025. Google's Gemini 2.5 Pro and OpenAI's GPT-5.1 were released after the initial knowledge cutoff, and the configuration uses correct current API model IDs.src/components/quiz/QuizInterface.tsx (1)
10-22: Prop wiring and passing-score fallback are consistentAccepting
coursePassingGradeinQuizInterfaceand resolvingpassingScoreascoursePassingGrade ?? contentItem?.passingScore ?? 70matches the shared grading behavior and keeps UI messaging aligned with course-level thresholds.src/components/learnspace/ai-tutor-panel.tsx (1)
114-134: Course passing grade is correctly propagated to quizzesDeriving
coursePassingGradefromactiveChapter.course?.passingGradeand passing it through toQuizzesPanelfor both text and video quizzes ensures quiz UIs follow the course-level threshold without changing existing branching logic.src/components/quiz/QuizResults.tsx (1)
19-33: Quiz results now correctly follow course-level passing gradeIntroducing
coursePassingGradeand resolving:const passingScore = coursePassingGrade ?? contentItem?.passingScore ?? 70; const passed = percentage >= passingScore;keeps this component’s pass/fail messaging and attempt-history badges aligned with the same precedence used in
QuizInterfaceand the grading utilities. This should prevent confusing discrepancies between what the UI shows as “passing” and what the course actually requires.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
convex/clerkAuth.ts (1)
196-217: Security fix looks good; consider minor simplification for consistency.Converting
getUserByClerkIdto aninternalQueryresolves the earlier concern about unauthenticated user lookup and PII exposure, since it’s no longer directly client-callable. Returning the fulluserobject is appropriate for internal server-side use.If you want to align with
getUserByEmailand simplify, you can drop the explicit null check and just return the query result:-export const getUserByClerkId = internalQuery({ - args: { clerkId: v.string() }, - handler: async (ctx, args) => { - const user = await ctx.db - .query("users") - .withIndex("by_clerk_id", (q) => q.eq("clerkId", args.clerkId)) - .first(); - - if (!user) { - return null; - } - - // Return full user info for internal server-side use - return user; - }, -}); +export const getUserByClerkId = internalQuery({ + args: { clerkId: v.string() }, + handler: async (ctx, args) => { + return ctx.db + .query("users") + .withIndex("by_clerk_id", (q) => q.eq("clerkId", args.clerkId)) + .first(); + }, +});src/components/landing/LandingPageClient.tsx (1)
225-233: Consider whether trust indicators convey clear value propositions.The trust indicators have been updated from benefit-focused statements ("No credit card required," "Works with any YouTube video") to feature-category labels ("YouTube Courses," "Capsule Courses," "Mastery Learning").
While this may align with your branding strategy, the new labels are less immediately informative about user benefits or trust signals. Consider whether first-time visitors will understand what these mean without additional context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
convex/clerkAuth.ts(1 hunks)package.json(1 hunks)src/app/api/auth/sync-role/route.ts(1 hunks)src/components/landing/LandingPageClient.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/api/auth/sync-role/route.ts
- package.json
🔇 Additional comments (3)
src/components/landing/LandingPageClient.tsx (3)
195-197: Grammar and branding issues resolved.The awkward phrasing from the previous version has been corrected to proper parallel structure, and YouTube is now consistently capitalized throughout the file. This addresses the issues flagged in past reviews.
211-216: Demo section intentionally removed.The Demo CTA has been commented out, simplifying the hero section to a single primary call-to-action. This is a deliberate content decision with no technical issues.
314-315: Copy updates improve consistency and accuracy.Multiple improvements throughout:
- Line 314: "Enroll in YouTube courses" with proper capitalization
- Line 327: "Interactive quizzes" with correct spelling (addresses past review comment about "quizes")
- Lines 496-497: More conservative claim removing "thousands of learners" quantification
All changes maintain consistency with the established voice and address previous feedback.
Also applies to: 327-328, 496-497
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.