Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds a full daily news quiz: Convex schema and APIs, a server-side generation action using OpenAI with strict schema validation and sanitization, scheduling and streak integration, a frontend /quiz route with grading/submission/share/review UI, navigation and i18n wiring (desktop and mobile), and several UI/type polish changes. ChangesDaily News Quiz Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 8
🤖 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 `@apps/web/src/routes/quiz.tsx`:
- Around line 357-380: The feedback container rendered when currentFeedback
exists should be made accessible by adding ARIA live region attributes to the
wrapping div (the element that currently uses cn(...) and renders the
CheckCircle2/XCircle and localize(currentFeedback.explanation, locale)); update
that div to include aria-live="polite" and role="status" (and optionally
aria-atomic="true") so screen readers will announce the success/error text when
currentFeedback changes.
In `@packages/backend/convex/interactions.ts`:
- Around line 813-817: The returned object currently exposes readCount but it
actually sums article views and quiz attempts; rename this field to a clearer
name like activityCount or dailyActivityCount in the return value (in the
function that currently returns { timestamp, readCount: (activeSet?.size ?? 0) +
quizCount, isToday }) and update all related type/interface definitions, usages,
and tests in the codebase (e.g., any callers expecting readCount) to the new
field name to keep types and runtime behavior consistent; alternatively, if you
prefer separate metrics, return both readCount: (activeSet?.size ?? 0) and
quizCount as distinct fields and update types/usages accordingly.
- Around line 708-713: The quizAttempts query currently collects all records via
ctx.db.query("quizAttempts").withIndex("by_user_date", (q) => q.eq("userId",
user._id)).order("desc").collect(), which can load unbounded history; restrict
it by either adding a take() (e.g., .take(200)) or by filtering to a recent date
range (compute a cutoff like 90 days ago and change the index/filter to
.withIndex("by_user_date", q => q.eq("userId", user._id).gte("date", cutoff)) or
equivalent) so the dashboard only fetches recent attempts (>=84 days) and avoids
loading thousands of rows.
In `@packages/backend/convex/quiz.ts`:
- Around line 117-157: getUserProfileByAuthUserId and
ensureUserProfileForAuthUser are duplicated here and in interactions.ts; extract
them into a single shared module (e.g., lib/userProfile.ts) that exports
getUserProfileByAuthUserId and ensureUserProfileForAuthUser, keep the same
signatures (using QueryCtx/MutationCtx and authUser shape) and logic (including
normalizeEmail and inserting userStats), then replace the local definitions in
both quiz.ts and interactions.ts with imports from that new module and update
callers to use the imported functions so there is one source of truth.
- Around line 159-205: The streak calculation in updateUserStatsForDailyQuiz
duplicates the logic found in updateUserStatsForView (interactions.ts); extract
that core logic into a shared helper (e.g., calculateNextStreak or
computeStreakUpdate) that accepts the current stats record and a timestamp
(completedAt), returns the new currentStreak, longestStreak delta or value, and
the updated lastActiveAt, then replace the duplicated block in
updateUserStatsForDailyQuiz and the corresponding block in
updateUserStatsForView to call this helper; ensure the helper lives in a common
util module, update imports, and preserve existing behavior (handling undefined
lastActiveAt, startOfUtcDay and DAY_MS comparisons) and unit tests.
- Around line 389-406: In the candidates.map async handler, avoid fetching
claims and articles until you confirm the event is published: first await
ctx.db.get(preview.eventId) into the existing event variable, check if (!event
|| event.status !== "published") and return null immediately, and only if
published run the two ctx.db.query(...) calls to load claims and articles;
update the Promise.all structure so the initial db.get is awaited separately
before running the parallel queries for "eventClaims" and "articles".
- Around line 306-315: The code currently returns the previous attempt when an
existingAttempt is found (using buildReview(quiz, existingAttempt.answers)),
which silently ignores the new args.answers; change this to either reject
duplicate submissions by throwing a ConvexError (e.g., throw new
ConvexError("You have already completed this quiz")) or explicitly update the
stored attempt with the new answers before returning (update existingAttempt
with args.answers and recompute buildReview(quiz, updatedAnswers)); locate the
handling around existingAttempt and buildReview in the quiz submission mutation
and implement one of these behaviors so the new submission is not silently
dropped.
In `@packages/backend/convex/quizNode.ts`:
- Line 285: Extract the magic number 0.45 used in the tokenOverlap check into a
named constant (e.g., TOKEN_OVERLAP_THRESHOLD) near the top of the module or the
surrounding function, add a brief comment explaining why 0.45 was chosen (or how
it was derived), and replace the literal in the tokenOverlap(normalizedClaim,
normalizedText) >= 0.45 expression with the constant; ensure the constant name
and comment are placed close to related helpers (e.g., tokenOverlap) so future
maintainers can find and adjust it.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fe4bd0a-6cd8-46d6-bce9-693a1f896827
⛔ Files ignored due to path filters (2)
apps/web/src/routeTree.gen.tsis excluded by!**/routeTree.gen.tspackages/backend/convex/_generated/api.d.tsis excluded by!**/_generated/**,!**/_generated/**
📒 Files selected for processing (14)
apps/web/src/components/header.tsxapps/web/src/components/layout/MobileTabBar.tsxapps/web/src/lib/i18n/strings.tsapps/web/src/routes/activitate.tsxapps/web/src/routes/feed.tsxapps/web/src/routes/quiz.tsxpackages/backend/convex/clustering.tspackages/backend/convex/crons.tspackages/backend/convex/enrichmentNode.tspackages/backend/convex/interactions.tspackages/backend/convex/lib/aiCall.tspackages/backend/convex/quiz.tspackages/backend/convex/quizNode.tspackages/backend/convex/schema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@apps/web/src/routes/quiz.tsx`:
- Around line 162-165: The component must not render the interactive quiz while
the Convex query existingAttempt (from useQuery(api.quiz.getMyTodayAttempt,
...)) is still loading (it is undefined for authenticated users); update the
render logic to gate the quiz UI: if isAuthenticated && existingAttempt ===
undefined, return a loading state (or null) instead of treating it as “no
attempt”. Apply this same guard wherever the code later assumes existingAttempt
(the block around the later quiz rendering/submit logic) so the UI only allows
answering/submitting once existingAttempt is resolved.
In `@packages/backend/convex/quiz.ts`:
- Around line 273-301: gradeQuizQuestion currently returns correctChoiceId
allowing clients to learn answers and then cheat when calling submitQuizAttempt;
remove correctChoiceId (and any other answer-key fields like isCorrect) from
gradeQuizQuestion's response and instead implement server-side grading inside
submitQuizAttempt (or require a persisted attempt record) so the server computes
correctness from the authoritative quiz data and the submitted answers. Update
gradeQuizQuestion to only return non-sensitive metadata (questionId,
explanation/attribution only if safe) and modify submitQuizAttempt to fetch the
quiz (from ctx.db.get using quizId), compare each submitted choice against
question.correctChoiceId on the server, compute score/streak, persist the
attempt, and ignore any client-supplied correctness fields.
- Around line 423-458: The replaceDailyQuiz handler currently overwrites an
existing ready quiz with a failed rerun; modify the handler in replaceDailyQuiz
so that when an existing row exists and existing.status === "ready" and
args.status === "failed" you do not clobber the published quiz data—either
return early (no patch) or patch only non-destructive fields (e.g., update
lastError and generatedAt but leave status, questions, and publishedAt intact).
Concretely, detect the condition after loading existing and before
building/applying row, and when it matches, skip updating
status/questions/publishedAt (or simply return { quizId: existing._id, replaced:
false } or similar), otherwise continue with the existing patch/insert logic;
reference replaceDailyQuiz, existing, row, publishedAt, questions, and lastError
when making the change.
- Around line 217-259: The server currently filters invalid answers into
normalizedAnswers but doesn't verify the caller answered every question once,
allowing partial submissions to be saved and block future attempts; after
computing normalizedAnswers (and/or before inserting into quizAttempts) check
that normalizedAnswers.length === quiz.questions.length and that every
questionId from quiz.questions is present exactly once (you can use
validQuestionIds and compare sets), and if not, return the same non-saved
response shape used for unauthenticated users (saved: false, completedAt:
Date.now(), quizId: quiz._id, dateKey: quiz.dateKey, ...result) instead of
inserting; ensure this check happens before creating existingAttempt/ inserting
into ctx.db.insert("quizAttempts") so partial attempts are never persisted.
In `@packages/backend/convex/quizNode.ts`:
- Around line 492-523: callOpenAI(...) can throw and currently that exception
bypasses the failure persistence; wrap the call to callOpenAI in a try/catch
around the existing callOpenAI<RawQuizResponse> invocation and in the catch
block call ctx.runMutation(internal.quiz.replaceDailyQuiz, { dateKey, status:
"failed", questions: [], sourceEventIds, inputSignature, model: settings.model,
lastError: errorOrMessage }) to persist the failed row (include the caught error
message/details), then return the same failure shape ({ status: "failed",
reason: errorOrMessage, questionCount: 0 }) or rethrow if desired; ensure you
reference callOpenAI, internal.quiz.replaceDailyQuiz, dateKey, inputSignature
and preserve existing fields (model, sourceEventIds) when recording the failure.
- Around line 399-431: The attribution fields are still taken directly from the
model output (raw.sourceNames[0], raw.sourceUrl) even though you validated
sources via eventSourceNames and computed sourceIds; change attribution to use
the validated event source instead: build a lookup of event.sources by _id (or
find the matching source by sourceIds[0]) and set attribution.sourceName and
attribution.sourceUrl from that event source (trimmed) or undefined if none,
rather than using raw.sourceNames/raw.sourceUrl; update the sanitized push where
attribution is set (reference: eventSourceNames, sourceIds, sanitized,
raw.sourceNames, raw.sourceUrl) to derive attribution from the matched event
source.
- Around line 589-604: Compute targetQuestions before minQuestions and ensure
minQuestions is clamped to not exceed targetQuestions; either call safeInteger
for minQuestions using targetQuestions as the max argument or, after both values
are computed, set minQuestions = Math.min(minQuestions, targetQuestions). Update
the QuizGenerationSettings construction (symbols: QuizGenerationSettings,
settings, targetQuestions, minQuestions, safeInteger,
quiz_generation_target_questions, quiz_generation_min_questions) so minQuestions
can never be greater than targetQuestions.
- Around line 386-409: The sanitization currently only rejects duplicate choice
text; add checks to reject duplicate question IDs and duplicate choice IDs: for
each raw, ensure that if raw.id exists it is not already seen by maintaining a
questionIds Set (and if raw.id is missing, generate an id and ensure it is
unique against questionIds), and ensure the set of raw.choices.map(c=>c.id) has
the same size as raw.choices.length to detect duplicate choice.id values; if
either duplicate-question-id or duplicate-choice-id is detected, skip/continue
and do not push to sanitized. Update code around choiceIds, raw.id, and the
point before sanitized.push to perform these checks.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 27149d19-8dfa-45e2-8dfd-6e8de7519aed
⛔ Files ignored due to path filters (1)
packages/backend/convex/_generated/api.d.tsis excluded by!**/_generated/**,!**/_generated/**
📒 Files selected for processing (7)
apps/web/src/components/streak-activity-calendar.tsxapps/web/src/routes/quiz.tsxpackages/backend/convex/interactions.tspackages/backend/convex/lib/streaks.tspackages/backend/convex/lib/userProfile.tspackages/backend/convex/quiz.tspackages/backend/convex/quizNode.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@apps/web/src/components/early-access-apply-card.tsx`:
- Around line 67-73: The onSubmit handler allows whitespace-only emails to
proceed; compute a normalized email (const normalizedEmail =
email.trim().toLowerCase()) inside the onSubmit, guard with if
(!normalizedEmail) { setMessage("Please enter a valid email"); return; } and
only then call addToWaitlist.mutate with email: normalizedEmail and name:
name.trim() || undefined; this prevents sending empty emails and provides user
feedback.
- Around line 108-115: The status paragraph that uses addToWaitlist.isError to
set role should also include an explicit aria-live attribute for consistent
screen reader announcements; update the <p> (the status region controlled by
addToWaitlist.isError) to add aria-live="assertive" when addToWaitlist.isError
is true and aria-live="polite" (or "off") when it's a non-error status,
preserving the existing role logic so screen readers receive correct live region
semantics.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b24ba1c-5e63-491f-bb34-bf3e56bf41a3
📒 Files selected for processing (16)
.vscode/tasks.jsonapps/web/src/components/SignInPrompt.tsxapps/web/src/components/early-access-apply-card.tsxapps/web/src/components/feed/event-card.tsxapps/web/src/components/sign-in-form.tsxapps/web/src/components/ui/page-loading-state.tsxapps/web/src/components/user-menu.tsxapps/web/src/lib/auth-redirect.tsapps/web/src/routes/event.$slug.tsxapps/web/src/routes/feed.tsxapps/web/src/routes/quiz.tsxapps/web/src/routes/source.$sourceId.tsxpackages/backend/convex/interactions.tspackages/backend/convex/lib/streaks.tspackages/backend/convex/quiz.tspackages/backend/convex/quizNode.ts
Merged
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
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.
Summary by CodeRabbit
New Features
UI
Localization
Backend