fix(#316): standardize database identity system on userEmail#562
Open
Krish-Makadiya wants to merge 9 commits into
Open
fix(#316): standardize database identity system on userEmail#562Krish-Makadiya wants to merge 9 commits into
Krish-Makadiya wants to merge 9 commits into
Conversation
…hema, API routes, Inngest workflows, and testing suite
|
CodeAnt AI is reviewing your PR. |
|
@Krish-Makadiya is attempting to deploy a commit to the Karan Mani Tripathi 's projects Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
gssoc'26
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (13)
WalkthroughThe PR standardizes identity by replacing ChangesEmail identity standardization
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 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 |
|
@coderabbitai review |
codeant-ai
Bot
added
the
size:L
github-actions
Bot
removed
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| .select({ doubtId: likesTable.doubtId }) | ||
| .from(likesTable) | ||
| .where(eq(likesTable.userName, userName)); | ||
| .where(eq(likesTable.userEmail, email)); |
There was a problem hiding this comment.
Suggestion: This lookup fetches all liked doubts for the user across the entire account history, even when the response is a single paginated slice. For heavy users this creates a growing per-request cost unrelated to the page size. Limit likes to the current page's doubt IDs (or use a join/subquery scoped to the fetched doubts) so response time scales with page size. [performance]
Severity Level: Major ⚠️
- ❌ GET /api/doubts listing scales with historical likes count.
- ⚠️ Heavy users see slower doubt list loading over time.Steps of Reproduction ✅
1. Start the application and authenticate as a user so that requests hit `GET(req:
Request)` in `src/app/api/doubts/route.ts:24-205`, which Next.js exposes at `GET
/api/doubts`.
2. Populate the `likesTable` with many rows for this user (e.g., by liking many doubts via
the UI or tests), so `likesTable.userEmail` matches the user's email across a large number
of records (imported at `src/app/api/doubts/route.ts:4-13`).
3. Trigger a paginated doubts fetch (e.g., open the doubts list page or call `GET
/api/doubts?page=1&limit=20`); inside `GET`, the code at
`src/app/api/doubts/route.ts:146-150` runs
`query.where(...).orderBy(...).limit(limit).offset(offset)` to fetch only one page of
doubts.
4. Immediately after fetching that single page, the branch at
`src/app/api/doubts/route.ts:152-156` executes:
`const userLikes = await db.select({ doubtId: likesTable.doubtId
}).from(likesTable).where(eq(likesTable.userEmail, email));`
which selects all likes for that `userEmail` across the entire `likesTable`, regardless
of the current page's doubt IDs, so for a heavy user the response cost grows with total
historical likes rather than the page size.Fix in Cursor | Fix in VSCode Claude
(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/route.ts
**Line:** 156:156
**Comment:**
*Performance: This lookup fetches all liked doubts for the user across the entire account history, even when the response is a single paginated slice. For heavy users this creates a growing per-request cost unrelated to the page size. Limit likes to the current page's doubt IDs (or use a join/subquery scoped to the fetched doubts) so response time scales with page size.
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| if (userIdentifier) { | ||
| const userUpvotes = await db.select().from(replyLikesTable).where(eq(replyLikesTable.userName, userIdentifier)); | ||
| if (email) { | ||
| const userUpvotes = await db.select().from(replyLikesTable).where(eq(replyLikesTable.userEmail, email)); |
There was a problem hiding this comment.
Suggestion: This query loads every reply-like ever made by the current user, even though this endpoint only needs vote state for replies belonging to one doubtId. As the user accumulates likes, this becomes an unbounded read on every replies fetch. Restrict the lookup to the current reply set (or join by doubtId) to keep the query bounded and avoid avoidable latency. [performance]
Severity Level: Major ⚠️
- ❌ GET /api/replies scales with all user reply likes.
- ⚠️ Opening DoubtRepliesModal becomes slower for active upvoters.Steps of Reproduction ✅
1. From the client, open a doubt's replies using `DoubtRepliesModal` in
`src/components/DoubtRepliesModal.tsx:32`; when `isOpen` is true, the `useEffect` at lines
66-73 calls `fetchReplies()` for that `doubt`.
2. `fetchReplies()` at `src/components/DoubtRepliesModal.tsx:117-130` issues
`fetch(`/api/replies?doubtId=${doubt.id}`)`, which Next.js routes to `GET(req: Request)`
in `src/app/api/replies/route.ts:14-85`.
3. In the replies GET handler, after validating the user and doubt, the code at
`src/app/api/replies/route.ts:65-68` loads only replies for the requested doubt:
`db.select().from(repliesTable).where(eq(repliesTable.doubtId, doubtId))`.
4. Because the user is authenticated and `email` is non-null, the block at
`src/app/api/replies/route.ts:70-73` runs:
`const userUpvotes = await
db.select().from(replyLikesTable).where(eq(replyLikesTable.userEmail, email));`
which loads every row in `replyLikesTable` for that user (all historical reply likes)
even though the endpoint only needs votes for replies under the current `doubtId`, so
for users who have liked many replies across different doubts, each
`/api/replies?doubtId=...` call performs an unbounded lookup unrelated to the current
thread.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/replies/route.ts
**Line:** 72:72
**Comment:**
*Performance: This query loads every reply-like ever made by the current user, even though this endpoint only needs vote state for replies belonging to one `doubtId`. As the user accumulates likes, this becomes an unbounded read on every replies fetch. Restrict the lookup to the current reply set (or join by `doubtId`) to keep the query bounded and avoid avoidable latency.
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
| // 7. Top Contributors (students who reply the most) | ||
| db.select({ | ||
| name: repliesTable.userName, | ||
| name: repliesTable.userEmail, |
There was a problem hiding this comment.
Suggestion: Returning contributor name directly from userEmail exposes personally identifiable email addresses in analytics responses and UI, leaking private user data to other classroom members. Use a non-sensitive display field (e.g., profile name or masked identifier) for this output. [security]
Severity Level: Critical 🚨
- ❌ Dashboard shows other students' raw emails as contributor names.
- ❌ Room "Top Contributors" section exposes classmates' email addresses.
- ⚠️ Analytics API leaks student emails to any classroom member.Steps of Reproduction ✅
1. Sign in with a Clerk account and open the dashboard page implemented in
`src/app/dashboard/page.tsx`, which on mount calls `fetch('/api/analytics')` and stores
the JSON in `data` (line ~62, confirmed by Grep).
2. That request is handled by `GET()` in `src/app/api/analytics/route.ts:8-15`, which
authenticates via `currentUser()`, checks classroom membership via
`membershipsTable.userEmail` (lines 24-48), and then runs the analytics queries including
the "Top Contributors" query.
3. In the "Top Contributors" query at `src/app/api/analytics/route.ts:133-146`, the code
selects `name: repliesTable.userEmail` (line 135) and groups by `repliesTable.userEmail`,
returning records where `name` is each contributor's raw email address.
4. On the dashboard, the "Contributors" card renders `data.topContributors` and displays
`contributor.name` directly in the UI at `src/app/dashboard/page.tsx:317-323`, so any
classroom member viewing the dashboard sees other contributors' full email addresses.
5. Similarly, in the classroom view at `src/app/rooms/[id]/page.tsx:911-933`, the "Top
Contributors" section iterates over `data.topContributors` and renders `contributor.name`
(line ~931) as uppercase text, again exposing each contributor's email to any user who can
load that classroom's analytics.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/analytics/route.ts
**Line:** 135:135
**Comment:**
*Security: Returning contributor `name` directly from `userEmail` exposes personally identifiable email addresses in analytics responses and UI, leaking private user data to other classroom members. Use a non-sensitive display field (e.g., profile name or masked identifier) for this output.
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| .values({ | ||
| doubtId: newDoubt.id, | ||
| userName: 'DoubtDesk AI', | ||
| userEmail: 'ai@doubtdesk.com', |
There was a problem hiding this comment.
Suggestion: Writing AI replies with a hardcoded userEmail that is not guaranteed to exist in usersTable will violate the foreign-key constraint on replies.userEmail and cause inserts to fail silently in this catch block. Ensure the AI identity is backed by a real users row (or use a non-FK system identity strategy) before inserting these rows. [api mismatch]
Severity Level: Critical 🚨
- ❌ First AI solution replies fail to persist in `replies` table.
- ❌ Doubts marked solved lack corresponding stored solution replies.
- ⚠️ "Recent AI replies" analytics return empty or incomplete data.Steps of Reproduction ✅
1. Apply the database schema from `src/configs/schema.ts` so that `repliesTable.userEmail`
is defined as `varchar(...).notNull()` with a foreign key `userEmailFk` referencing
`usersTable.email` (lines 214-237), enforcing that every reply's `userEmail` must
correspond to a real row in `users`.
2. Run the application without creating a `users` row for `ai@doubtdesk.com` (the
hardcoded AI identity used only in code; Grep shows occurrences only in
`analytics/route.ts`, `teacher/analytics/route.ts`, and `ask-ai/route.ts`, with no seeding
logic).
3. As a logged-in user whose email exists in `usersTable`, send a first-time POST request
to `/api/ask-ai` handled by `POST()` in `src/app/api/ask-ai/route.ts:201-588` with a
prompt and optional `classroomId` such that `isFollowUp` is `false` (no conversation
history).
4. Inside the `if (!isFollowUp)` persistence block at
`src/app/api/ask-ai/route.ts:505-566`, the code inserts a new doubt with `userEmail:
email` (lines 507-523), then attempts to insert a reply into `repliesTable` with
`userEmail: 'ai@doubtdesk.com'` (line 534). Because no corresponding
`users.email='ai@doubtdesk.com'` exists, the database rejects this insert due to the
foreign key constraint, the error is caught and only logged by the catch block at lines
560-564, and the API still returns a successful JSON response with the AI `reply` while no
`replies` row is created and `doubts.solvedReplyId` is never updated.
5. Subsequent features that rely on persisted AI replies—such as "Recent AI replies"
querying `eq(repliesTable.userEmail, 'ai@doubtdesk.com')` in
`src/app/api/analytics/route.ts:148-167` or any UI expecting a stored solution tied to
`solvedReplyId`—observe missing data because those AI replies were never inserted.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/app/api/ask-ai/route.ts
**Line:** 534:534
**Comment:**
*Api Mismatch: Writing AI replies with a hardcoded `userEmail` that is not guaranteed to exist in `usersTable` will violate the foreign-key constraint on `replies.userEmail` and cause inserts to fail silently in this catch block. Ensure the AI identity is backed by a real users row (or use a non-FK system identity strategy) before inserting these rows.
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
Comment on lines
+242
to
272
| userEmail: varchar({ length: 255 }).notNull(), | ||
| doubtId: integer().notNull(), | ||
| createdAt: timestamp().defaultNow().notNull(), | ||
| }, (table) => ({ | ||
| doubtIdFk: foreignKey({ | ||
| columns: [table.doubtId], | ||
| foreignColumns: [doubtsTable.id], | ||
| }).onDelete("cascade"), | ||
| userNameDoubtUnique: unique("likes_userName_doubtId_unique").on(table.userName, table.doubtId), | ||
| userEmailFk: foreignKey({ | ||
| columns: [table.userEmail], | ||
| foreignColumns: [usersTable.email], | ||
| }).onDelete("cascade"), | ||
| userEmailDoubtUnique: unique("likes_userEmail_doubtId_unique").on(table.userEmail, table.doubtId), | ||
| })); | ||
|
|
||
| export const replyLikesTable = pgTable("reply_likes", { | ||
| id: integer().primaryKey().generatedAlwaysAsIdentity(), | ||
| userName: varchar({ length: 255 }).notNull(), | ||
| userEmail: varchar({ length: 255 }).notNull(), | ||
| replyId: integer().notNull(), | ||
| createdAt: timestamp().defaultNow().notNull(), | ||
| }, (table) => ({ | ||
| replyIdFk: foreignKey({ | ||
| columns: [table.replyId], | ||
| foreignColumns: [repliesTable.id], | ||
| }).onDelete("cascade"), | ||
| userNameReplyUnique: unique("reply_likes_userName_replyId_unique").on(table.userName, table.replyId), | ||
| userEmailFk: foreignKey({ | ||
| columns: [table.userEmail], | ||
| foreignColumns: [usersTable.email], | ||
| }).onDelete("cascade"), | ||
| userEmailReplyUnique: unique("reply_likes_userEmail_replyId_unique").on(table.userEmail, table.replyId), | ||
| })); |
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Core identity columns were changed in the ORM schema (likes.userEmail, reply_likes.userEmail, and stricter doubts.userEmail / replies.userEmail), but the Drizzle SQL migrations still define/expect the old userName-based schema. In a migrated database, the likes and reply_likes tables will not have the userEmail columns that the runtime code now uses, so like/unlike operations will issue SQL against non-existent columns and fail at runtime.
Suggestion: Add forward migrations that rename/backfill userName -> userEmail for likes and reply_likes, adjust constraints/FKs accordingly, and update the Drizzle journal so deployed databases match src/configs/schema.ts. Ensure the migrations cover existing data and preserve uniqueness semantics before relying on the new schema in production.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is an **Architect / Logical Review** comment left during a code review. These reviews are first-class, important findings — not optional suggestions. Do NOT dismiss this as a 'big architectural change' just because the title says architect review; most of these can be resolved with a small, localized fix once the intent is understood.
**Path:** src/configs/schema.ts
**Line:** 242:272
**Comment:**
*HIGH: Core identity columns were changed in the ORM schema (`likes.userEmail`, `reply_likes.userEmail`, and stricter `doubts.userEmail` / `replies.userEmail`), but the Drizzle SQL migrations still define/expect the old `userName`-based schema. In a migrated database, the `likes` and `reply_likes` tables will not have the `userEmail` columns that the runtime code now uses, so like/unlike operations will issue SQL against non-existent columns and fail at runtime.
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.
If a suggested approach is provided above, use it as the authoritative instruction. If no explicit code suggestion is given, you MUST still draft and apply your own minimal, localized fix — do not punt back with 'no suggestion provided, review manually'. Keep the change as small as possible: add a guard clause, gate on a loading state, reorder an await, wrap in a conditional, etc. Do not refactor surrounding code or expand scope beyond the finding.
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
|
CodeAnt AI finished reviewing your PR. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
src/types/index.ts (1)
199-214:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake
Doubt.userEmailandReply.userEmailrequired to match the schema.These interfaces still allow
null/undefined, but the schema now defines both columns asNOT NULL. Leaving them optional weakens the contract and lets missing email propagation slip through type-checking.Suggested fix
export interface Doubt { id: number; - - userEmail?: string | null; + userEmail: string; classroomId?: number | null; @@ export interface Reply { id: number; doubtId: number; - - userEmail?: string | null; + userEmail: string; type: "comment" | "solution";Also applies to: 225-235
🤖 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 `@src/types/index.ts` around lines 199 - 214, The Doubt and Reply interfaces allow userEmail to be optional/null but the DB schema marks those columns NOT NULL; update the interfaces so Doubt.userEmail and Reply.userEmail are required non-nullable strings (remove the optional modifier and the | null union) to match the schema and prevent missing-email propagation through type-checking; locate the declarations for interface Doubt and interface Reply in src/types/index.ts and change their userEmail types accordingly.src/configs/schema.ts (1)
145-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
userEmailFK delete behavior with the newNOT NULLcontract.
doubts.userEmailandreplies.userEmailare nownotNull(), but both foreign keys still useonDelete("set null"). Deleting a referencedusers.emailrow will now hit an impossible constraint path instead of applying the FK action cleanly. Pick a consistent policy here: either keep these columns nullable, or switch the FK tocascade/restrict.Suggested fix
userEmailFk: foreignKey({ columns: [table.userEmail], foreignColumns: [usersTable.email], - }).onDelete("set null"), + }).onDelete("cascade"),Apply the same change to both
doubtsTableandrepliesTableunless you intentionally want to preserve records after user deletion, in which case the columns need to become nullable again.Also applies to: 217-237
🤖 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 `@src/configs/schema.ts` around lines 145 - 167, The foreign key onDelete policy for userEmail conflicts with the column being notNull; update the FK actions to match the NOT NULL contract by replacing .onDelete("set null") with .onDelete("cascade") on the userEmail foreignKey definitions for both the doubts table and the replies table (the foreignKey created for table.userEmail that references usersTable.email), so deleting a user will cascade delete related doubts/replies; alternatively, if you want to preserve those rows, make the userEmail column nullable (remove .notNull()) in both doubtsTable and repliesTable—apply the chosen change consistently to both places.src/app/api/ask-ai/route.ts (1)
530-565:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnsure the AI actor email is guaranteed to satisfy FK constraints.
Using a hardcoded
userEmail: 'ai@doubtdesk.com'can fail inserts if that account is not present inusersTable, and the surroundingcatchmasks this as a partial-write success. This risks storing the doubt without its solution linkage.Suggested fix
+const AI_SYSTEM_EMAIL = "ai@doubtdesk.com"; + // inside the persistence block before inserting into repliesTable +const [aiUser] = await db + .select({ email: usersTable.email }) + .from(usersTable) + .where(eq(usersTable.email, AI_SYSTEM_EMAIL)) + .limit(1); + +if (!aiUser) { + throw new Error(`Missing AI system user: ${AI_SYSTEM_EMAIL}`); +} + const [aiReply] = await db .insert(repliesTable) .values({ doubtId: newDoubt.id, - userEmail: 'ai@doubtdesk.com', + userEmail: AI_SYSTEM_EMAIL, type: 'solution', content: reply,🤖 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 `@src/app/api/ask-ai/route.ts` around lines 530 - 565, The insert uses a hardcoded userEmail ('ai@doubtdesk.com') which may violate the usersTable FK and leave a partially persisted doubt; before inserting into repliesTable (and before updating doubtsTable), ensure the AI user exists and run these DB operations inside a single transaction: check usersTable for the AI user (by email), create/insert it if absent (returning its identifying column), then use that confirmed user record/email when inserting into repliesTable and updating doubtsTable so FK constraints cannot fail mid-flow; perform the repliesTable insert + doubtsTable update within a db.transaction (or the project's atomic API) so any failure rolls back the doubt and reply together.src/app/api/replies/route.ts (1)
149-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t use email as the reply display-name fallback.
All three paths now expose
user.fullName || emailto other users. When a replier has no full name configured, their raw email becomes the notification name, which is an unnecessary PII leak caused by the migration. Use a safe fallback string or a dedicated public profile field instead.Proposed fix
- replierName: user.fullName || email, + replierName: user.fullName || "Someone",Also applies to: 188-197, 224-230
🤖 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 `@src/app/api/replies/route.ts` around lines 149 - 156, The notification payload currently exposes the replier's raw email as a display name by using user.fullName || email in createReplyNotification calls; change the fallback to a non-PII safe value (e.g., a dedicated public profile field like user.publicDisplayName || "Anonymous" or just "A student") instead of email. Update all occurrences where replierName is set (the createReplyNotification call around newReply[0].id and the other two sites noted in this file) to use the safe fallback or public profile property (replierName: user.publicDisplayName || "Anonymous") so emails are never leaked in notifications.src/app/api/doubts/route.ts (1)
264-270:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid exposing raw email in classroom notifications.
authorName: user.fullName || emailleaks the poster’s email address to other classroom members whenever their profile has no full name. That’s a privacy regression introduced by the identity change; use a non-PII fallback instead.Proposed fix
createClassroomDoubtNotifications({ classroomId: parsedClassroomId, doubtId: newDoubt.id, subject, authorEmail: email, - authorName: user.fullName || email, + authorName: user.fullName || "Someone", doubtType: doubtType }).catch((err) => console.error("Notification trigger async failure:", 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 `@src/app/api/doubts/route.ts` around lines 264 - 270, The notification currently sets authorName to user.fullName || email which leaks users' emails; update the call to createClassroomDoubtNotifications to use a non-PII fallback instead (e.g., authorName: user.fullName || 'Anonymous' or 'A student') so that when user.fullName is missing the email value is not exposed; ensure the change is applied where createClassroomDoubtNotifications is invoked (look for parsedClassroomId, newDoubt.id, subject, authorEmail: email, authorName) and keep authorEmail separate for internal use only.src/components/DoubtRepliesModal.tsx (1)
117-129:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface reply-load failures instead of falling through to the empty state.
When this fetch fails, the code only logs and clears
isLoading, so the modal can incorrectly render “No interactions yet.” after a network/server error. Add a dedicated error state here and render a retry path for the main thread load. As per coding guidelines,**/*.tsx: Review for: Missing loading/error states.🤖 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 `@src/components/DoubtRepliesModal.tsx` around lines 117 - 129, The fetchReplies error handling currently only logs and clears loading, causing the UI to fall through to the empty state; add a dedicated error state (e.g., repliesError and setRepliesError) in the DoubtRepliesModal component, set repliesError with the caught error inside the catch of fetchReplies, and avoid clobbering existing replies on failure; expose a retry path by wiring a retry button in the modal to call fetchReplies again (use the same doubt.id), and update the render logic to show a clear error message + retry control when repliesError is set instead of showing “No interactions yet.” Ensure setIsLoading(false) is still called in finally.src/__tests__/api/doubts.test.ts (1)
106-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe
popularandunsolvedcases are asserting behavior the DB mock never simulates.
db.select()always falls back tomockDoubtsin the original order, and the chain methods ignorewhere/orderBy, so these tests fail exactly the way CI reports. Either make the mock apply the requested filtering/sorting, or assert the route's query-builder calls instead of the returned array order/content.Also applies to: 153-180
🤖 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 `@src/__tests__/api/doubts.test.ts` around lines 106 - 122, The tests for the "popular" and "unsolved" routes fail because the db.select mock (db.select, createChainWithData, createEmptyChain, mockDoubts) always returns mockDoubts in original order and does not honor where/orderBy, so update the mock to simulate query-builder behavior (implement where and orderBy on the returned chain to filter by solved/unsolved and to sort by reply count/likes using the supplied fields) or alternatively change the tests to assert the query-builder calls on db.select (e.g., that select(...) then where(...) and orderBy(...) were invoked with expected args) instead of asserting on returned array ordering/content; apply the same fix for the other related tests around lines 153-180 that exercise popular/unsolved behavior.src/lib/offline/syncQueue.ts (1)
139-159:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t replace the queued identity with
"Pending User"once the UI now keys ownership offuserEmail.
DoubtCardandDoubtRepliesModalboth compare the current Clerk email against these projected records. With this placeholder, every offline-created doubt/reply becomes “not mine” until sync finishes, which breaks the ownership model this PR is standardizing. Preserve the submitting user's email in the queued payload, or add a separate display-only field instead of overloadinguserEmail.🤖 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 `@src/lib/offline/syncQueue.ts` around lines 139 - 159, The mapping that projects queued items in getPendingReplies and the earlier pending-doubts mapper incorrectly overwrites the submitter identity by setting userEmail to "Pending User"; update both projection sites (the .map that creates id: `pending-${item.id}` objects and getPendingReplies' .map) to preserve the original submitting email from the queued payload (e.g., use item.payload.userEmail or item.payload.submittedByEmail) and only fall back to a display-only placeholder if that field is missing, or alternatively add a new field like displayName/displayUser for UI presentation so you do not mutate userEmail which the ownership checks (DoubtCard/DoubtRepliesModal) rely on. Ensure the change is applied to both pending-doubt and pending-reply projections (the functions/mappers constructing id: `pending-${item.id}` objects).
🤖 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 `@src/__tests__/api/replies-vote.test.ts`:
- Around line 70-73: The test seeds a self-vote because selectResultQueue's
mocked selected row uses userEmail: 'teacher@example.com' (the acting user),
causing the route to block and return 403; change the mocked reply owner in
selectResultQueue (the object inside the second array) to a different email
(e.g., 'author@example.com') so the reply is not owned by the Clerk user, and
keep the vote row insert assertions using 'teacher@example.com' so the test
exercises the insert path in the vote handling logic (look for selectResultQueue
and the mocked row with id/replyId/userEmail in replies-vote.test.ts).
In `@src/app/api/analytics/route.ts`:
- Around line 135-145: The current query exposes raw contributor emails by
selecting repliesTable.userEmail as the display name; change it to keep
userEmail for grouping but return a non-PII display label: either join the
users/profile table to select users.displayName (or COALESCE(users.displayName,
...)) as name, or select the masked email (e.g., SQL expression/substr + '***' +
domain) in the projection instead of repliesTable.userEmail; keep GROUP BY and
WHERE using repliesTable.userEmail but ensure the final selected field for the
API is a non-PII column (e.g., name or maskedEmail) so topContributors does not
leak student emails.
In `@src/app/api/doubts/action/`[id]/route.ts:
- Line 17: The destructuring of the incoming payload in route.ts omitted status,
but later the solve path reads data.status; update the destructuring of data
(where const { action, content, subject, imageUrl, replyId, tags = [] } = data)
to also include status (e.g. , status) so the solve branch and type-checking can
access the explicit status value from the validated payload; ensure the status
variable name matches how it’s consumed in the solve flow.
In `@src/app/api/doubts/route.ts`:
- Around line 74-78: The search branch currently builds searchCondition only
against doubtsTable.content and doubtsTable.subject, dropping the prior "search
by author" behavior; to fix, preserve author matching by joining the
user/profile display field and including an ilike condition against it (e.g.
join the users or profiles table used elsewhere and add ilike(users.display,
`%${search}%`) to the or(...) expression). Update the query in route.ts that
constructs searchCondition to perform the necessary join with the user/profile
table (same table referenced elsewhere in this module) and include the
display-name ilike predicate so clients sending author queries continue to
match.
In `@src/components/DoubtCard.tsx`:
- Around line 181-185: The UI currently derives the avatar initial and label
from doubt.userEmail (seen in the span and h3 where doubt.userEmail?.charAt(0)
and doubt.userEmail?.split('@')[0] are used); change these to use a public
display value instead: prefer doubt.displayName if provided, otherwise render an
anonymized alias (e.g., "User #<shortId>" or a deterministic pseudonym from
doubt.id) and use only the first character of that display name for the avatar;
implement or call a small helper (getPublicDisplayName or similar) to centralize
this logic and update the span and h3 to use that helper, keeping userEmail
exclusively for internal storage/logging.
- Around line 53-57: The effect that sets isOwner only sets it to true and never
resets it, so when props/auth change the owner UI can remain visible; update the
useEffect in DoubtCard to compute the ownership boolean and call setIsOwner with
the full expression (e.g., setIsOwner(isSignedIn &&
user?.primaryEmailAddress?.emailAddress === doubt.userEmail)) so it toggles
true/false on every change of isSignedIn, user, or doubt.userEmail; ensure the
dependencies remain [isSignedIn, user, doubt.userEmail].
In `@src/components/DoubtRepliesModal.tsx`:
- Around line 66-74: The useEffect only sets isDoubtOwner to true on the happy
path which can leave the flag stale; inside the useEffect for when isOpen
changes (the block that calls fetchReplies), explicitly reset or compute
isDoubtOwner for every run instead of only setting true — e.g., call
setIsDoubtOwner(false) before the conditional or replace the conditional with
setIsDoubtOwner(isSignedIn && user?.primaryEmailAddress?.emailAddress ===
doubt.userEmail) so the flag is cleared when the current user is not the doubt
owner; update the effect that references useEffect, fetchReplies, isOpen,
isSignedIn, user, doubt.userEmail, and setIsDoubtOwner accordingly.
In `@src/inngest/functions.ts`:
- Around line 176-177: The digest currently exposes PII by setting replierName
to repliesTable.userEmail; change this to a safe display label instead: look up
the replier's display name (e.g., usersTable.displayName) when available,
otherwise derive a sanitized fallback from the email (e.g., masked or local-part
only with masking) and assign that to replierName; update both occurrences where
replierName is set (the lines using repliesTable.userEmail and the second
occurrence at the other block) so outbound digests never directly include
repliesTable.userEmail but use the display name or sanitized fallback.
---
Outside diff comments:
In `@src/__tests__/api/doubts.test.ts`:
- Around line 106-122: The tests for the "popular" and "unsolved" routes fail
because the db.select mock (db.select, createChainWithData, createEmptyChain,
mockDoubts) always returns mockDoubts in original order and does not honor
where/orderBy, so update the mock to simulate query-builder behavior (implement
where and orderBy on the returned chain to filter by solved/unsolved and to sort
by reply count/likes using the supplied fields) or alternatively change the
tests to assert the query-builder calls on db.select (e.g., that select(...)
then where(...) and orderBy(...) were invoked with expected args) instead of
asserting on returned array ordering/content; apply the same fix for the other
related tests around lines 153-180 that exercise popular/unsolved behavior.
In `@src/app/api/ask-ai/route.ts`:
- Around line 530-565: The insert uses a hardcoded userEmail
('ai@doubtdesk.com') which may violate the usersTable FK and leave a partially
persisted doubt; before inserting into repliesTable (and before updating
doubtsTable), ensure the AI user exists and run these DB operations inside a
single transaction: check usersTable for the AI user (by email), create/insert
it if absent (returning its identifying column), then use that confirmed user
record/email when inserting into repliesTable and updating doubtsTable so FK
constraints cannot fail mid-flow; perform the repliesTable insert + doubtsTable
update within a db.transaction (or the project's atomic API) so any failure
rolls back the doubt and reply together.
In `@src/app/api/doubts/route.ts`:
- Around line 264-270: The notification currently sets authorName to
user.fullName || email which leaks users' emails; update the call to
createClassroomDoubtNotifications to use a non-PII fallback instead (e.g.,
authorName: user.fullName || 'Anonymous' or 'A student') so that when
user.fullName is missing the email value is not exposed; ensure the change is
applied where createClassroomDoubtNotifications is invoked (look for
parsedClassroomId, newDoubt.id, subject, authorEmail: email, authorName) and
keep authorEmail separate for internal use only.
In `@src/app/api/replies/route.ts`:
- Around line 149-156: The notification payload currently exposes the replier's
raw email as a display name by using user.fullName || email in
createReplyNotification calls; change the fallback to a non-PII safe value
(e.g., a dedicated public profile field like user.publicDisplayName ||
"Anonymous" or just "A student") instead of email. Update all occurrences where
replierName is set (the createReplyNotification call around newReply[0].id and
the other two sites noted in this file) to use the safe fallback or public
profile property (replierName: user.publicDisplayName || "Anonymous") so emails
are never leaked in notifications.
In `@src/components/DoubtRepliesModal.tsx`:
- Around line 117-129: The fetchReplies error handling currently only logs and
clears loading, causing the UI to fall through to the empty state; add a
dedicated error state (e.g., repliesError and setRepliesError) in the
DoubtRepliesModal component, set repliesError with the caught error inside the
catch of fetchReplies, and avoid clobbering existing replies on failure; expose
a retry path by wiring a retry button in the modal to call fetchReplies again
(use the same doubt.id), and update the render logic to show a clear error
message + retry control when repliesError is set instead of showing “No
interactions yet.” Ensure setIsLoading(false) is still called in finally.
In `@src/configs/schema.ts`:
- Around line 145-167: The foreign key onDelete policy for userEmail conflicts
with the column being notNull; update the FK actions to match the NOT NULL
contract by replacing .onDelete("set null") with .onDelete("cascade") on the
userEmail foreignKey definitions for both the doubts table and the replies table
(the foreignKey created for table.userEmail that references usersTable.email),
so deleting a user will cascade delete related doubts/replies; alternatively, if
you want to preserve those rows, make the userEmail column nullable (remove
.notNull()) in both doubtsTable and repliesTable—apply the chosen change
consistently to both places.
In `@src/lib/offline/syncQueue.ts`:
- Around line 139-159: The mapping that projects queued items in
getPendingReplies and the earlier pending-doubts mapper incorrectly overwrites
the submitter identity by setting userEmail to "Pending User"; update both
projection sites (the .map that creates id: `pending-${item.id}` objects and
getPendingReplies' .map) to preserve the original submitting email from the
queued payload (e.g., use item.payload.userEmail or
item.payload.submittedByEmail) and only fall back to a display-only placeholder
if that field is missing, or alternatively add a new field like
displayName/displayUser for UI presentation so you do not mutate userEmail which
the ownership checks (DoubtCard/DoubtRepliesModal) rely on. Ensure the change is
applied to both pending-doubt and pending-reply projections (the
functions/mappers constructing id: `pending-${item.id}` objects).
In `@src/types/index.ts`:
- Around line 199-214: The Doubt and Reply interfaces allow userEmail to be
optional/null but the DB schema marks those columns NOT NULL; update the
interfaces so Doubt.userEmail and Reply.userEmail are required non-nullable
strings (remove the optional modifier and the | null union) to match the schema
and prevent missing-email propagation through type-checking; locate the
declarations for interface Doubt and interface Reply in src/types/index.ts and
change their userEmail types accordingly.
🪄 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: CHILL
Plan: Pro
Run ID: 391aaa77-bbde-4939-a3d4-1bfcbf17a008
📒 Files selected for processing (28)
src/__tests__/api/doubts.test.tssrc/__tests__/api/replies-action.test.tssrc/__tests__/api/replies-auto-transition.test.tssrc/__tests__/api/replies-vote.test.tssrc/__tests__/components/DoubtCard.test.tsxsrc/app/api/analytics/route.tssrc/app/api/ask-ai/route.tssrc/app/api/bookmarks/route.tssrc/app/api/doubts/[id]/upvote/route.tssrc/app/api/doubts/action/[id]/route.tssrc/app/api/doubts/route.tssrc/app/api/inngest/ConfusionSpikeDetector.tssrc/app/api/karma/route.tssrc/app/api/profile/route.tssrc/app/api/replies/route.tssrc/app/api/replies/vote/route.tssrc/app/api/teacher/analytics/route.tssrc/components/DoubtCard.tsxsrc/components/DoubtRepliesModal.tsxsrc/components/InfiniteDoubtFeed.tsxsrc/configs/schema.tssrc/inngest/functions.tssrc/inngest/karma.tssrc/lib/offline/syncQueue.tssrc/lib/profile/getProfileStats.tssrc/lib/validations/doubt.tssrc/lib/validations/reply.tssrc/types/index.ts
💤 Files with no reviewable changes (2)
- src/app/api/karma/route.ts
- src/inngest/karma.ts
Comment on lines
53
to
+57
| useEffect(() => { | ||
| const savedName = localStorage.getItem("anonymous_user"); | ||
| if (savedName === doubt.userName) { | ||
| if (isSignedIn && user?.primaryEmailAddress?.emailAddress === doubt.userEmail) { | ||
| setIsOwner(true); | ||
| } | ||
| }, [doubt.userName]); | ||
| }, [isSignedIn, user, doubt.userEmail]); |
Contributor
There was a problem hiding this comment.
Reset isOwner on every auth/prop change.
This effect only promotes the state to true. If the card is reused for a different doubt, or the user signs out / switches accounts, owner-only controls can stay visible until remount.
💡 Proposed fix
- useEffect(() => {
- if (isSignedIn && user?.primaryEmailAddress?.emailAddress === doubt.userEmail) {
- setIsOwner(true);
- }
- }, [isSignedIn, user, doubt.userEmail]);
+ useEffect(() => {
+ setIsOwner(
+ Boolean(
+ isSignedIn &&
+ user?.primaryEmailAddress?.emailAddress === doubt.userEmail,
+ ),
+ );
+ }, [isSignedIn, user?.primaryEmailAddress?.emailAddress, doubt.userEmail]);🤖 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 `@src/components/DoubtCard.tsx` around lines 53 - 57, The effect that sets
isOwner only sets it to true and never resets it, so when props/auth change the
owner UI can remain visible; update the useEffect in DoubtCard to compute the
ownership boolean and call setIsOwner with the full expression (e.g.,
setIsOwner(isSignedIn && user?.primaryEmailAddress?.emailAddress ===
doubt.userEmail)) so it toggles true/false on every change of isSignedIn, user,
or doubt.userEmail; ensure the dependencies remain [isSignedIn, user,
doubt.userEmail].
Comment on lines
+181
to
+185
| <span className="text-lg font-black text-blue-400">{doubt.userEmail?.charAt(0)?.toUpperCase() || '?'}</span> | ||
| </div> | ||
| <div> | ||
| <h3 className="text-slate-900 dark:text-white font-bold tracking-tight text-sm"> | ||
| {doubt.userName || 'Anonymous'} | ||
| {doubt.userEmail?.split('@')[0] || 'Anonymous'} |
Contributor
There was a problem hiding this comment.
Keep userEmail as an internal identifier, not the public display name.
Deriving the avatar and label from the email local-part leaks private identity data in the feed. The DB can standardize on userEmail, but the UI should render an explicit display name or an anonymized alias instead.
🤖 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 `@src/components/DoubtCard.tsx` around lines 181 - 185, The UI currently
derives the avatar initial and label from doubt.userEmail (seen in the span and
h3 where doubt.userEmail?.charAt(0) and doubt.userEmail?.split('@')[0] are
used); change these to use a public display value instead: prefer
doubt.displayName if provided, otherwise render an anonymized alias (e.g., "User
#<shortId>" or a deterministic pseudonym from doubt.id) and use only the first
character of that display name for the avatar; implement or call a small helper
(getPublicDisplayName or similar) to centralize this logic and update the span
and h3 to use that helper, keeping userEmail exclusively for internal
storage/logging.
Comment on lines
66
to
74
| useEffect(() => { | ||
| if (isOpen) { | ||
| fetchReplies(); | ||
| const savedName = localStorage.getItem("anonymous_user"); | ||
| if (savedName) setUserName(savedName); | ||
| if (savedName === doubt.userName) setIsDoubtOwner(true); | ||
| if (isSignedIn && user?.primaryEmailAddress?.emailAddress === doubt.userEmail) { | ||
| setIsDoubtOwner(true); | ||
| } | ||
| } | ||
| }, [isOpen, doubt.id, doubt.userName]); | ||
| }, [isOpen, doubt.id, doubt.userEmail, isSignedIn, user]); | ||
|
|
Contributor
There was a problem hiding this comment.
Reset isDoubtOwner instead of only setting it on the happy path.
This state is only flipped to true, so reopening the modal for a different doubt or after an auth change can leave owner-only actions enabled for the wrong thread.
💡 Proposed fix
- useEffect(() => {
- if (isOpen) {
- fetchReplies();
- if (isSignedIn && user?.primaryEmailAddress?.emailAddress === doubt.userEmail) {
- setIsDoubtOwner(true);
- }
- }
- }, [isOpen, doubt.id, doubt.userEmail, isSignedIn, user]);
+ useEffect(() => {
+ if (isOpen) {
+ fetchReplies();
+ }
+
+ setIsDoubtOwner(
+ Boolean(
+ isOpen &&
+ isSignedIn &&
+ user?.primaryEmailAddress?.emailAddress === doubt.userEmail,
+ ),
+ );
+ }, [isOpen, doubt.id, doubt.userEmail, isSignedIn, user?.primaryEmailAddress?.emailAddress]);📝 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
| useEffect(() => { | |
| if (isOpen) { | |
| fetchReplies(); | |
| const savedName = localStorage.getItem("anonymous_user"); | |
| if (savedName) setUserName(savedName); | |
| if (savedName === doubt.userName) setIsDoubtOwner(true); | |
| if (isSignedIn && user?.primaryEmailAddress?.emailAddress === doubt.userEmail) { | |
| setIsDoubtOwner(true); | |
| } | |
| } | |
| }, [isOpen, doubt.id, doubt.userName]); | |
| }, [isOpen, doubt.id, doubt.userEmail, isSignedIn, user]); | |
| useEffect(() => { | |
| if (isOpen) { | |
| fetchReplies(); | |
| } | |
| setIsDoubtOwner( | |
| Boolean( | |
| isOpen && | |
| isSignedIn && | |
| user?.primaryEmailAddress?.emailAddress === doubt.userEmail, | |
| ), | |
| ); | |
| }, [isOpen, doubt.id, doubt.userEmail, isSignedIn, user?.primaryEmailAddress?.emailAddress]); |
🤖 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 `@src/components/DoubtRepliesModal.tsx` around lines 66 - 74, The useEffect
only sets isDoubtOwner to true on the happy path which can leave the flag stale;
inside the useEffect for when isOpen changes (the block that calls
fetchReplies), explicitly reset or compute isDoubtOwner for every run instead of
only setting true — e.g., call setIsDoubtOwner(false) before the conditional or
replace the conditional with setIsDoubtOwner(isSignedIn &&
user?.primaryEmailAddress?.emailAddress === doubt.userEmail) so the flag is
cleared when the current user is not the doubt owner; update the effect that
references useEffect, fetchReplies, isOpen, isSignedIn, user, doubt.userEmail,
and setIsDoubtOwner accordingly.
Comment on lines
+176
to
177
| replierName: repliesTable.userEmail, | ||
| replyContent: repliesTable.content, |
Contributor
There was a problem hiding this comment.
Don’t expose raw email addresses as digest “replierName”.
replierName: repliesTable.userEmail pushes PII into outbound digests. Keep identity internal as email, but transform to a safe display label before emailing (e.g., profile display name or sanitized local-part).
Suggested fix
- doubtsMap.get(p.doubtId)!.replies.push({
- replierName: p.replierName,
+ const safeName = p.replierName?.includes("@")
+ ? p.replierName.split("@")[0]
+ : (p.replierName || "Someone");
+
+ doubtsMap.get(p.doubtId)!.replies.push({
+ replierName: safeName,
content: p.replyContent || ""
});Also applies to: 257-258
🤖 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 `@src/inngest/functions.ts` around lines 176 - 177, The digest currently
exposes PII by setting replierName to repliesTable.userEmail; change this to a
safe display label instead: look up the replier's display name (e.g.,
usersTable.displayName) when available, otherwise derive a sanitized fallback
from the email (e.g., masked or local-part only with masking) and assign that to
replierName; update both occurrences where replierName is set (the lines using
repliesTable.userEmail and the second occurrence at the other block) so outbound
digests never directly include repliesTable.userEmail but use the display name
or sanitized fallback.
knoxiboy
added
backend
Owner
|
Hi @Krish-Makadiya! Thanks for your work on this. Before we can merge, please address the following:
Once these are fixed, the PR will be automatically evaluated again. Let us know if you need any help! |
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
the
size:L
github-actions
Bot
removed
the
size:L
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/api/doubts/action/[id]/route.ts (1)
49-60: ⚡ Quick winDuplicate membership query—consolidate with the earlier access check.
Lines 35-37 already fetch the membership row to verify classroom access. The identical query is repeated here (lines 50-58) just to evaluate teacher status. Reusing the first result avoids a redundant database round-trip.
Suggested refactor
- // Security: Verify doubt visibility/classroom membership + // Security: Verify doubt visibility/classroom membership & determine teacher status + let membership: typeof membershipsTable.$inferSelect | undefined; if (doubt.classroomId && email) { - const [membership] = await db.select().from(membershipsTable).where( + [membership] = await db.select().from(membershipsTable).where( and(eq(membershipsTable.userEmail, email), eq(membershipsTable.classroomId, doubt.classroomId)) ); if (!membership) { return NextResponse.json({ error: "Access denied to this classroom's doubt" }, { status: 403 }); } } else if (doubt.classroomId && !email) { return NextResponse.json({ error: "Unauthorized access to classroom doubt" }, { status: 401 }); } // Permission check for sensitive actions const isOwner = email && doubt.userEmail === email; - let isTeacher = false; - - if (doubt.classroomId && email) { - const [membership] = await db - .select() - .from(membershipsTable) - .where( - and( - eq(membershipsTable.userEmail, email), - eq(membershipsTable.classroomId, doubt.classroomId) - ) - ); - - isTeacher = !!(membership && canTeach(membership.role)); - } + const isTeacher = !!(membership && canTeach(membership.role));🤖 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 `@src/app/api/doubts/action/`[id]/route.ts around lines 49 - 60, The code runs a second DB query to fetch the classroom membership just to set isTeacher; instead reuse the membership row already retrieved during the earlier access check instead of re-querying membershipsTable. Remove the duplicate const [membership] = await db... block and use the previously fetched membership variable (or rename the earlier result to membership) to set isTeacher = !!(membership && canTeach(membership.role)) when doubt.classroomId && email are present. Ensure the existing membership variable is in scope where isTeacher is assigned.
🤖 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 `@src/app/api/doubts/action/`[id]/route.ts:
- Line 18: The handler destructures status from incoming data but the validation
schema updateDoubtActionSchema in src/lib/validations/doubt.ts doesn't declare
status, causing the TypeScript build failure; update updateDoubtActionSchema to
include a status field (matching the expected type — e.g., optional string or a
constrained enum/union of allowed statuses) and ensure its TypeScript return
type reflects that addition so the route's const { action, content, subject,
imageUrl, replyId, status, tags = [] } = data passes validation and type-checks.
---
Nitpick comments:
In `@src/app/api/doubts/action/`[id]/route.ts:
- Around line 49-60: The code runs a second DB query to fetch the classroom
membership just to set isTeacher; instead reuse the membership row already
retrieved during the earlier access check instead of re-querying
membershipsTable. Remove the duplicate const [membership] = await db... block
and use the previously fetched membership variable (or rename the earlier result
to membership) to set isTeacher = !!(membership && canTeach(membership.role))
when doubt.classroomId && email are present. Ensure the existing membership
variable is in scope where isTeacher is assigned.
🪄 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: CHILL
Plan: Pro
Run ID: 9c88b722-41cf-4099-a77d-8e312941a38e
📒 Files selected for processing (9)
src/__tests__/api/doubts.test.tssrc/app/api/analytics/route.tssrc/app/api/ask-ai/route.tssrc/app/api/doubts/action/[id]/route.tssrc/app/api/doubts/route.tssrc/app/api/inngest/ConfusionSpikeDetector.tssrc/app/api/replies/route.tssrc/app/api/teacher/analytics/route.tssrc/configs/schema.ts
💤 Files with no reviewable changes (1)
- src/app/api/inngest/ConfusionSpikeDetector.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tests/api/doubts.test.ts
- src/app/api/teacher/analytics/route.ts
- src/app/api/analytics/route.ts
- src/app/api/ask-ai/route.ts
- src/app/api/replies/route.ts
- src/configs/schema.ts
- src/app/api/doubts/route.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/api/doubts/route.ts (1)
93-99:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAuthor search now matches email instead of display name.
The search condition on line 97 uses
doubtsTable.userEmail, which means users must search by email address (e.g., "john@example.com") rather than display name (e.g., "John Smith"). This significantly degrades UX for author-based searches.Per issue
#316's objectives, the migration should "standardize on userEmail" for identity storage, but user-facing search should still support display names by joining theusersTableand matching against the name field.Verify whether users table has a searchable name/display field:
#!/bin/bash # Description: Check usersTable schema for display name fields echo "=== Checking usersTable schema definition ===" rg -A 20 "export const usersTable = pgTable" src/configs/schema.ts echo "" echo "=== Looking for user name/display usage in queries ===" rg -n "usersTable\.(name|displayName|fullName)" src/app/api/ --type tsIf a name field exists, restore author search by joining:
// Example fix (adjust to actual schema) if (search) { const searchCondition = or( ilike(doubtsTable.content, `%${search}%`), ilike(doubtsTable.subject, `%${search}%`), ilike(doubtsTable.userEmail, `%${search}%`), // Add join-based name search sql`EXISTS ( SELECT 1 FROM users WHERE users.email = ${doubtsTable.userEmail} AND users.name ILIKE ${`%${search}%`} )` ); }🤖 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 `@src/app/api/doubts/route.ts` around lines 93 - 99, The current searchCondition in route.ts uses doubtsTable.userEmail which forces email-only author matches; update the condition to include a search against the users table display/name field by adding a join or EXISTS subquery that checks usersTable.name (or usersTable.displayName/fullName if present) ILIKE `%search%` while keeping the existing content, subject and userEmail checks; first verify the usersTable schema for the exact name field, then modify the searchBranch that builds searchCondition (the or(...) expression around ilike(doubtsTable.content,...), ilike(doubtsTable.subject,...), ilike(doubtsTable.userEmail,...)) to include a usersTable-based name match (via SQL EXISTS or a proper join using usersTable.email = doubtsTable.userEmail) so user-facing searches match display names.
🧹 Nitpick comments (1)
src/app/api/ask-ai/route.ts (1)
661-666: ⚡ Quick winMove AI system user initialization to migration or app startup.
The AI system user insertion runs on every POST request. While
onConflictDoNothingprevents duplicate key errors, this still incurs unnecessary query overhead on every AI interaction.Move this to a one-time initialization:
♻️ Option 1: Add to migration SQL
-- In drizzle/0010_identity_system_update.sql or a new migration INSERT INTO "users" ("name", "email", "role") VALUES ('DoubtDesk AI', 'ai@doubtdesk.com', 'system') ON CONFLICT ("email") DO NOTHING;♻️ Option 2: Add to app initialization
Create a setup function called once at startup:
// src/lib/init.ts export async function ensureSystemUsers() { await db.insert(usersTable).values({ name: 'DoubtDesk AI', email: 'ai@doubtdesk.com', role: 'system' }).onConflictDoNothing({ target: usersTable.email }); } // In app startup or middleware await ensureSystemUsers();🤖 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 `@src/app/api/ask-ai/route.ts` around lines 661 - 666, The insertion of the AI system user currently runs on every POST in route.ts (the db.insert into usersTable for 'DoubtDesk AI'), causing unnecessary DB overhead; move this into a one-time initialization instead by creating a startup helper (e.g., ensureSystemUsers) that performs the insert with onConflictDoNothing and call it during app startup or add the INSERT into your migrations; update route.ts to remove the db.insert block so the request handler no longer attempts to create the AI user on each request.
🤖 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 `@drizzle/0010_identity_system_update.sql`:
- Around line 1-2: The migration renames likes.userName → likes.userEmail and
reply_likes.userName → reply_likes.userEmail but never ensures those values are
valid emails that match users.email before re-creating FK constraints; add a
data backfill/validation step that maps or rejects non-email usernames prior to
renaming and FK creation. Specifically, before ALTER TABLE ... RENAME COLUMN for
likes.userName and reply_likes.userName, run a step that (a) validates format or
joins to users on username to find the correct users.email, (b) updates
likes.userName and reply_likes.userName with the corresponding users.email where
possible, and (c) either fails the migration with a clear error or logs/stores
unmapped rows for manual reconciliation if any rows cannot be mapped to
users.email; then proceed to rename the columns and recreate FKs to
users(email).
- Around line 10-14: The migration adds FK constraints
likes_userEmail_users_email_fk and reply_likes_userEmail_users_email_fk on
likes.userEmail and reply_likes.userEmail referencing users.email without
validating data; update 0010_identity_system_update.sql to first check for and
handle orphaned rows in likes and reply_likes (rows where userEmail is not
present in users.email)—either raise an explicit error or delete/repair those
rows—before executing ALTER TABLE ... ADD CONSTRAINT so the FK creation cannot
fail silently or leave inconsistent data.
---
Duplicate comments:
In `@src/app/api/doubts/route.ts`:
- Around line 93-99: The current searchCondition in route.ts uses
doubtsTable.userEmail which forces email-only author matches; update the
condition to include a search against the users table display/name field by
adding a join or EXISTS subquery that checks usersTable.name (or
usersTable.displayName/fullName if present) ILIKE `%search%` while keeping the
existing content, subject and userEmail checks; first verify the usersTable
schema for the exact name field, then modify the searchBranch that builds
searchCondition (the or(...) expression around ilike(doubtsTable.content,...),
ilike(doubtsTable.subject,...), ilike(doubtsTable.userEmail,...)) to include a
usersTable-based name match (via SQL EXISTS or a proper join using
usersTable.email = doubtsTable.userEmail) so user-facing searches match display
names.
---
Nitpick comments:
In `@src/app/api/ask-ai/route.ts`:
- Around line 661-666: The insertion of the AI system user currently runs on
every POST in route.ts (the db.insert into usersTable for 'DoubtDesk AI'),
causing unnecessary DB overhead; move this into a one-time initialization
instead by creating a startup helper (e.g., ensureSystemUsers) that performs the
insert with onConflictDoNothing and call it during app startup or add the INSERT
into your migrations; update route.ts to remove the db.insert block so the
request handler no longer attempts to create the AI user on each request.
🪄 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: CHILL
Plan: Pro
Run ID: 4ff0872c-8679-46aa-898b-ec3ff097ec5a
📒 Files selected for processing (6)
drizzle/0010_identity_system_update.sqldrizzle/meta/_journal.jsonsrc/__tests__/api/replies-vote.test.tssrc/app/api/analytics/route.tssrc/app/api/ask-ai/route.tssrc/app/api/doubts/route.ts
✅ Files skipped from review due to trivial changes (1)
- drizzle/meta/_journal.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tests/api/replies-vote.test.ts
- src/app/api/analytics/route.ts
Contributor
Author
|
Made the required changes, ready to get merge, so i can pick the next issues |
knoxiboy
approved these changes
Jun 7, 2026
Owner
|
@Krish-Makadiya we are ready to merge just resolve merge conflicts |
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
the
size:L
github-actions
Bot
removed
the
size:L
|
CodeAnt AI Incremental review completed. |
Contributor
Author
|
Solved the conflicts!! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/profile/route.ts (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore soft-delete guard in profile doubts query.
Line 27 currently includes soft-deleted doubts, which inflates
stats.totalDoubts/helpfulVotesand leaks deleted activity back intoactivities.doubts.Suggested fix
- db.select().from(doubtsTable).where(eq(doubtsTable.userEmail, email)), + db.select() + .from(doubtsTable) + .where(and( + eq(doubtsTable.userEmail, email), + isNull(doubtsTable.deletedAt) + )),🤖 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 `@src/app/api/profile/route.ts` at line 27, The query selecting doubts uses db.select().from(doubtsTable).where(eq(doubtsTable.userEmail, email)) and currently returns soft-deleted rows; update that where clause to also filter out soft-deleted records (e.g., add a condition that doubtsTable.deletedAt IS NULL or doubtsTable.deleted === false) so only non-deleted doubts are counted—modify the db.select(...).where(...) call to include this additional soft-delete guard alongside eq(doubtsTable.userEmail, email).src/components/DoubtCard.tsx (1)
307-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame accessibility issue: generic
aria-labelon bookmark button.Replace with a descriptive label consistent with the
titleattribute.♿ Proposed fix
- aria-label="Interactive button" + aria-label={doubt.hasBookmarked ? "Remove bookmark" : "Add to bookmarks"}As per coding guidelines:
**/*.tsxfiles should be reviewed for "Accessibility (aria labels, keyboard navigation)".🤖 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 `@src/components/DoubtCard.tsx` at line 307, In the DoubtCard component, the bookmark button uses a generic aria-label ("Interactive button"); change this to a descriptive label matching the element's title attribute (e.g., "Bookmark question" or "Remove bookmark" depending on state) so screen readers get the same text as the title; update the JSX for the bookmark button element (the button that currently has aria-label="Interactive button" and a title prop) to set aria-label equal to the title or to a localized equivalent and ensure state-based text (bookmark vs. unbookmark) is used if the button toggles.Source: Coding guidelines
🤖 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 `@src/components/DoubtCard.tsx`:
- Line 35: The DoubtCard component is missing destructuring of the disableModal
and onCommentClick props, causing TypeScript errors; update the DoubtCard
function signature to include disableModal and onCommentClick (from
DoubtCardProps) so the component uses the passed props consistently wherever
disableModal and onCommentClick are referenced (e.g., inside the component body
where onCommentClick is called and disableModal is checked); ensure the
parameter list matches other props (doubt, onUpdate, onViewAISolution, role,
openRepliesOnMount) and that any usages of disableModal/onCommentClick refer to
these destructured variables.
- Around line 173-177: The handleShare function calls
navigator.clipboard.writeText without awaiting or handling failures; update
handleShare (the function referencing doubt.id and
navigator.clipboard.writeText) to use the promise returned by writeText (await
or .then/.catch) and show a failure toast (e.g., toast.error) if it rejects, and
optionally provide a fallback (e.g., selecting a hidden input or informing the
user to copy manually) so users receive feedback on both success and error
cases.
- Line 215: The button in the DoubtCard component uses a non-descriptive
aria-label ("Interactive button"); update the button element in DoubtCard.tsx to
use a descriptive, state-aware aria-label that matches the title (e.g.,
aria-label={isPinned ? "Unpin doubt" : "Pin doubt to top"}), using the same
state/prop that controls the visual pin (e.g., isPinned or pinned) so screen
readers receive the correct action text; if you use i18n/localized strings,
reference those same keys for both title and aria-label.
---
Outside diff comments:
In `@src/app/api/profile/route.ts`:
- Line 27: The query selecting doubts uses
db.select().from(doubtsTable).where(eq(doubtsTable.userEmail, email)) and
currently returns soft-deleted rows; update that where clause to also filter out
soft-deleted records (e.g., add a condition that doubtsTable.deletedAt IS NULL
or doubtsTable.deleted === false) so only non-deleted doubts are counted—modify
the db.select(...).where(...) call to include this additional soft-delete guard
alongside eq(doubtsTable.userEmail, email).
In `@src/components/DoubtCard.tsx`:
- Line 307: In the DoubtCard component, the bookmark button uses a generic
aria-label ("Interactive button"); change this to a descriptive label matching
the element's title attribute (e.g., "Bookmark question" or "Remove bookmark"
depending on state) so screen readers get the same text as the title; update the
JSX for the bookmark button element (the button that currently has
aria-label="Interactive button" and a title prop) to set aria-label equal to the
title or to a localized equivalent and ensure state-based text (bookmark vs.
unbookmark) is used if the button toggles.
🪄 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: CHILL
Plan: Pro
Run ID: 45f30df4-b890-4fe1-9208-e027deefc6dd
📒 Files selected for processing (18)
drizzle/0010_identity_system_update.sqlsrc/__tests__/api/doubts.test.tssrc/app/api/analytics/route.tssrc/app/api/ask-ai/route.tssrc/app/api/bookmarks/route.tssrc/app/api/doubts/[id]/upvote/route.tssrc/app/api/doubts/action/[id]/route.tssrc/app/api/doubts/route.tssrc/app/api/inngest/ConfusionSpikeDetector.tssrc/app/api/profile/route.tssrc/app/api/replies/route.tssrc/app/api/teacher/analytics/route.tssrc/components/DoubtCard.tsxsrc/components/DoubtRepliesModal.tsxsrc/configs/schema.tssrc/lib/offline/syncQueue.tssrc/lib/validations/doubt.tssrc/lib/validations/reply.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/validations/reply.ts
- src/app/api/bookmarks/route.ts
- src/lib/validations/doubt.ts
- src/lib/offline/syncQueue.ts
- src/tests/api/doubts.test.ts
- src/app/api/analytics/route.ts
- src/app/api/inngest/ConfusionSpikeDetector.ts
- src/app/api/doubts/route.ts
- src/app/api/doubts/action/[id]/route.ts
- src/app/api/replies/route.ts
- src/app/api/ask-ai/route.ts
- src/components/DoubtRepliesModal.tsx
- src/configs/schema.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/api/profile/route.ts (1)
27-27:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore soft-delete guard in profile doubts query.
Line 27 currently includes soft-deleted doubts, which inflates
stats.totalDoubts/helpfulVotesand leaks deleted activity back intoactivities.doubts.Suggested fix
- db.select().from(doubtsTable).where(eq(doubtsTable.userEmail, email)), + db.select() + .from(doubtsTable) + .where(and( + eq(doubtsTable.userEmail, email), + isNull(doubtsTable.deletedAt) + )),🤖 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 `@src/app/api/profile/route.ts` at line 27, The query selecting doubts uses db.select().from(doubtsTable).where(eq(doubtsTable.userEmail, email)) and currently returns soft-deleted rows; update that where clause to also filter out soft-deleted records (e.g., add a condition that doubtsTable.deletedAt IS NULL or doubtsTable.deleted === false) so only non-deleted doubts are counted—modify the db.select(...).where(...) call to include this additional soft-delete guard alongside eq(doubtsTable.userEmail, email).src/components/DoubtCard.tsx (1)
307-307:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame accessibility issue: generic
aria-labelon bookmark button.Replace with a descriptive label consistent with the
titleattribute.♿ Proposed fix
- aria-label="Interactive button" + aria-label={doubt.hasBookmarked ? "Remove bookmark" : "Add to bookmarks"}As per coding guidelines:
**/*.tsxfiles should be reviewed for "Accessibility (aria labels, keyboard navigation)".🤖 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 `@src/components/DoubtCard.tsx` at line 307, In the DoubtCard component, the bookmark button uses a generic aria-label ("Interactive button"); change this to a descriptive label matching the element's title attribute (e.g., "Bookmark question" or "Remove bookmark" depending on state) so screen readers get the same text as the title; update the JSX for the bookmark button element (the button that currently has aria-label="Interactive button" and a title prop) to set aria-label equal to the title or to a localized equivalent and ensure state-based text (bookmark vs. unbookmark) is used if the button toggles.Source: Coding guidelines
🤖 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 `@src/components/DoubtCard.tsx`:
- Line 35: The DoubtCard component is missing destructuring of the disableModal
and onCommentClick props, causing TypeScript errors; update the DoubtCard
function signature to include disableModal and onCommentClick (from
DoubtCardProps) so the component uses the passed props consistently wherever
disableModal and onCommentClick are referenced (e.g., inside the component body
where onCommentClick is called and disableModal is checked); ensure the
parameter list matches other props (doubt, onUpdate, onViewAISolution, role,
openRepliesOnMount) and that any usages of disableModal/onCommentClick refer to
these destructured variables.
- Around line 173-177: The handleShare function calls
navigator.clipboard.writeText without awaiting or handling failures; update
handleShare (the function referencing doubt.id and
navigator.clipboard.writeText) to use the promise returned by writeText (await
or .then/.catch) and show a failure toast (e.g., toast.error) if it rejects, and
optionally provide a fallback (e.g., selecting a hidden input or informing the
user to copy manually) so users receive feedback on both success and error
cases.
- Line 215: The button in the DoubtCard component uses a non-descriptive
aria-label ("Interactive button"); update the button element in DoubtCard.tsx to
use a descriptive, state-aware aria-label that matches the title (e.g.,
aria-label={isPinned ? "Unpin doubt" : "Pin doubt to top"}), using the same
state/prop that controls the visual pin (e.g., isPinned or pinned) so screen
readers receive the correct action text; if you use i18n/localized strings,
reference those same keys for both title and aria-label.
---
Outside diff comments:
In `@src/app/api/profile/route.ts`:
- Line 27: The query selecting doubts uses
db.select().from(doubtsTable).where(eq(doubtsTable.userEmail, email)) and
currently returns soft-deleted rows; update that where clause to also filter out
soft-deleted records (e.g., add a condition that doubtsTable.deletedAt IS NULL
or doubtsTable.deleted === false) so only non-deleted doubts are counted—modify
the db.select(...).where(...) call to include this additional soft-delete guard
alongside eq(doubtsTable.userEmail, email).
In `@src/components/DoubtCard.tsx`:
- Line 307: In the DoubtCard component, the bookmark button uses a generic
aria-label ("Interactive button"); change this to a descriptive label matching
the element's title attribute (e.g., "Bookmark question" or "Remove bookmark"
depending on state) so screen readers get the same text as the title; update the
JSX for the bookmark button element (the button that currently has
aria-label="Interactive button" and a title prop) to set aria-label equal to the
title or to a localized equivalent and ensure state-based text (bookmark vs.
unbookmark) is used if the button toggles.
🪄 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: CHILL
Plan: Pro
Run ID: 45f30df4-b890-4fe1-9208-e027deefc6dd
📒 Files selected for processing (18)
drizzle/0010_identity_system_update.sqlsrc/__tests__/api/doubts.test.tssrc/app/api/analytics/route.tssrc/app/api/ask-ai/route.tssrc/app/api/bookmarks/route.tssrc/app/api/doubts/[id]/upvote/route.tssrc/app/api/doubts/action/[id]/route.tssrc/app/api/doubts/route.tssrc/app/api/inngest/ConfusionSpikeDetector.tssrc/app/api/profile/route.tssrc/app/api/replies/route.tssrc/app/api/teacher/analytics/route.tssrc/components/DoubtCard.tsxsrc/components/DoubtRepliesModal.tsxsrc/configs/schema.tssrc/lib/offline/syncQueue.tssrc/lib/validations/doubt.tssrc/lib/validations/reply.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/lib/validations/reply.ts
- src/app/api/bookmarks/route.ts
- src/lib/validations/doubt.ts
- src/lib/offline/syncQueue.ts
- src/tests/api/doubts.test.ts
- src/app/api/analytics/route.ts
- src/app/api/inngest/ConfusionSpikeDetector.ts
- src/app/api/doubts/route.ts
- src/app/api/doubts/action/[id]/route.ts
- src/app/api/replies/route.ts
- src/app/api/ask-ai/route.ts
- src/components/DoubtRepliesModal.tsx
- src/configs/schema.ts
🛑 Comments failed to post (3)
src/components/DoubtCard.tsx (3)
35-35:
⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing destructuring of
disableModalandonCommentClickprops causes TypeScript compilation failure.The props
disableModalandonCommentClickare defined inDoubtCardProps(lines 31-32) and used throughout the component (lines 338-340, 375-380, 402), but they are not destructured in the function signature. This breaks TypeScript compilation.🐛 Proposed fix
-export default function DoubtCard({ doubt, onUpdate, onViewAISolution, role, openRepliesOnMount = false }: DoubtCardProps) { +export default function DoubtCard({ doubt, onUpdate, onViewAISolution, role, openRepliesOnMount = false, disableModal = false, onCommentClick }: DoubtCardProps) {📝 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.export default function DoubtCard({ doubt, onUpdate, onViewAISolution, role, openRepliesOnMount = false, disableModal = false, onCommentClick }: DoubtCardProps) {🤖 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 `@src/components/DoubtCard.tsx` at line 35, The DoubtCard component is missing destructuring of the disableModal and onCommentClick props, causing TypeScript errors; update the DoubtCard function signature to include disableModal and onCommentClick (from DoubtCardProps) so the component uses the passed props consistently wherever disableModal and onCommentClick are referenced (e.g., inside the component body where onCommentClick is called and disableModal is checked); ensure the parameter list matches other props (doubt, onUpdate, onViewAISolution, role, openRepliesOnMount) and that any usages of disableModal/onCommentClick refer to these destructured variables.Source: Linters/SAST tools
173-177:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
handleSharelacks error handling for clipboard API.
navigator.clipboard.writeText()can fail (e.g., permissions denied, non-secure context). Users would receive no feedback on failure.🛡️ Proposed fix
const handleShare = () => { const url = `${window.location.origin}/doubts/${doubt.id}`; - navigator.clipboard.writeText(url); - toast.success("Link copied!"); + navigator.clipboard.writeText(url) + .then(() => toast.success("Link copied!")) + .catch(() => toast.error("Failed to copy link")); };As per coding guidelines:
**/*.tsxfiles should be reviewed for "Missing loading/error states".📝 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.const handleShare = () => { const url = `${window.location.origin}/doubts/${doubt.id}`; navigator.clipboard.writeText(url) .then(() => toast.success("Link copied!")) .catch(() => toast.error("Failed to copy link")); };🤖 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 `@src/components/DoubtCard.tsx` around lines 173 - 177, The handleShare function calls navigator.clipboard.writeText without awaiting or handling failures; update handleShare (the function referencing doubt.id and navigator.clipboard.writeText) to use the promise returned by writeText (await or .then/.catch) and show a failure toast (e.g., toast.error) if it rejects, and optionally provide a fallback (e.g., selecting a hidden input or informing the user to copy manually) so users receive feedback on both success and error cases.Source: Coding guidelines
215-215:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNon-descriptive
aria-labelreduces accessibility.The
aria-label="Interactive button"doesn't convey the button's purpose to assistive technology users. Use a descriptive label like "Pin doubt to top" or "Unpin doubt" (thetitleattribute already has this text).♿ Proposed fix
- aria-label="Interactive button" + aria-label={doubt.isPinned ? "Unpin doubt" : "Pin doubt to top"}As per coding guidelines:
**/*.tsxfiles should be reviewed for "Accessibility (aria labels, keyboard navigation)".📝 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.aria-label={doubt.isPinned ? "Unpin doubt" : "Pin doubt to top"}🤖 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 `@src/components/DoubtCard.tsx` at line 215, The button in the DoubtCard component uses a non-descriptive aria-label ("Interactive button"); update the button element in DoubtCard.tsx to use a descriptive, state-aware aria-label that matches the title (e.g., aria-label={isPinned ? "Unpin doubt" : "Pin doubt to top"}), using the same state/prop that controls the visual pin (e.g., isPinned or pinned) so screen readers receive the correct action text; if you use i18n/localized strings, reference those same keys for both title and aria-label.Source: Coding guidelines
Owner
|
Automated Detailed Review
Please address the above items and request a review again. |
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
2 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.
User description
Description
Closes #316
This PR addresses the critical identity system fragmentation where different tables used a mix of random
userNamestrings, Clerk user IDs, anduserEmail. It standardizes the entire application to useuserEmailas the single source of truth for identity, enabling reliable cross-referencing for user activity views.Schema Changes
likes&reply_likestables: RenameduserNametouserEmail, added foreign keys referencingusersTable, and updated unique constraints.doubts&repliestables: Removed theuserNamecolumn entirely and made the existinguserEmailcolumn strictlynotNull.API & Worker Updates
userEmailfrom Clerk'scurrentUser()instead of relying on client-provideduserNamevalues.inngest/functions.tsandinngest/ConfusionSpikeDetector.tsto rely onuserEmailfor fetching doubt authors and repliers.UI & Component Updates
DoubtCardandDoubtRepliesModal: RemoveduserNamedependency fromlocalStorageand switched to utilizinguseUser()from@clerk/nextjsto accurately fetch the user's primary email address.userEmailschema.Testing
mockDoubtshapes and mock database builders in test files (doubts.test.ts,replies-action.test.ts,replies-auto-transition.test.ts,replies-vote.test.ts, andDoubtCard.test.tsx).tsc --noEmitvalidation against the newly structured schema.Summary by CodeRabbit
Bug Fixes
Chores
CodeAnt-AI Description
Standardize doubts and replies around email-based identity
What Changed
Impact
✅ Fewer missing likes and upvotes✅ Correct reply ownership and delete access✅ More accurate profile and classroom activity stats💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.