feat: add share menu for doubts#620
Conversation
|
CodeAnt AI is reviewing your PR. |
|
@bhumishah2411 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
size/m
gssoc'26
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a share-dropdown to DoubtCard (copy, WhatsApp, Telegram). Backend tweaks: use drizzle ChangesSharing Enhancements
Doubts API (counting/pagination)
Replies API (membership gating)
Database migration & schema snapshot
Home page cleanup
Config fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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:M
github-actions
Bot
removed
the
size:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| <button | ||
| className="flex items-center justify-center p-3 rounded-2xl bg-white/5 hover:bg-white/10 text-slate-400 hover:text-white border border-white/5 transition-all outline-none" | ||
| title="Share doubt" | ||
| aria-label="Share doubt" |
There was a problem hiding this comment.
🎨 Design Review — HIGH
Do you think removing the trigger's default outline (outline-none) without adding a replacement focus style might make keyboard focus hard to perceive for the new Share menu button?
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a **Design Review** comment — a question about the UX/design of frontend code. It is intentionally framed as a question, not a prescription. The author may agree or disagree.
**Path:** src/components/DoubtCard.tsx
**Line:** 327:330
**Comment:**
*HIGH: Do you think removing the trigger's default outline (`outline-none`) without adding a replacement focus style might make keyboard focus hard to perceive for the new Share menu button?
- If you agree with the proposal, apply a small, localized change (swap a color token, bump a font size, adjust spacing, add an aria-label, etc.).
- If you disagree, or the answer depends on a design decision a human should make, explain your reasoning and ask the user how to proceed.
Do NOT refactor surrounding components or apply other design changes that weren't asked about.
| navigator.clipboard.writeText(getShareUrl()); | ||
| toast.success("Link copied!"); |
There was a problem hiding this comment.
Suggestion: The copy action shows success immediately without waiting for clipboard.writeText to resolve, so users can see "Link copied!" even when permission is denied or clipboard access fails. Make this handler async, await the write call, and show an error toast on rejection. [logic error]
Severity Level: Major ⚠️
- ⚠️ Share menu misreports copy success on clipboard failure.
- ⚠️ Users may rely on uncopied doubt links.Steps of Reproduction ✅
1. Navigate to any page that renders `DoubtCard`, for example `/bookmarks` implemented in
`src/app/bookmarks/page.tsx` lines 116-120, where each `bookmarks.map` item renders
`<DoubtCard key={doubt.id} doubt={doubt} onUpdate={fetchBookmarks} role={appUser?.role}
/>`.
2. In the UI, on any doubt card, click the share icon button defined in
`src/components/DoubtCard.tsx` lines 66-73 (footer actions share button inside the
`DropdownMenuTrigger`), which opens the share dropdown.
3. Click the "Copy Link" menu item wired to `onClick={handleShare}` in
`src/components/DoubtCard.tsx` lines 77-80, which invokes `handleShare`.
4. When `handleShare` executes (defined at `src/components/DoubtCard.tsx` lines 175-178),
it calls `navigator.clipboard.writeText(getShareUrl());` (line 176) but does not `await`
or wrap it in `try/catch`, and immediately calls `toast.success("Link copied!");` (line
177); in any environment where `navigator.clipboard.writeText` rejects or throws (e.g.,
clipboard permission denied or unsupported), the user still sees a success toast even
though the link was never copied, and the error is unhandled.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/components/DoubtCard.tsx
**Line:** 176:177
**Comment:**
*Logic Error: The copy action shows success immediately without waiting for `clipboard.writeText` to resolve, so users can see "Link copied!" even when permission is denied or clipboard access fails. Make this handler async, await the write call, and show an error toast on rejection.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix|
|
||
| const handleWhatsAppShare = () => { | ||
| const text = encodeURIComponent(`Check out this doubt on DoubtDesk:\n\n${getShareUrl()}`); | ||
| window.open(`https://api.whatsapp.com/send?text=${text}`, "_blank"); |
There was a problem hiding this comment.
Suggestion: Opening an external URL with window.open(..., "_blank") without noopener/noreferrer lets the new tab access window.opener, which enables reverse-tabnabbing attacks. Open the window with noopener,noreferrer (or equivalent safe link behavior) to prevent opener access. [security]
Severity Level: Major ⚠️
- ❌ Malicious WhatsApp page can redirect DoubtDesk tab.
- ⚠️ Users sharing doubts risk phishing via tab takeover.Steps of Reproduction ✅
1. Navigate to any page that renders `DoubtCard`, such as `/public-rooms` implemented in
`src/app/public-rooms/page.tsx` lines 7-11, where `filteredDoubts.map` renders `<DoubtCard
key={`${doubt.id}-${index}`} doubt={doubt} onUpdate={() => mutate()} />`.
2. On a doubt card, click the share icon button defined in `src/components/DoubtCard.tsx`
lines 66-73 to open the share dropdown.
3. Click the "Share on WhatsApp" menu item defined in `src/components/DoubtCard.tsx` lines
81-84, which has `onClick={handleWhatsAppShare}`.
4. The `handleWhatsAppShare` function in `src/components/DoubtCard.tsx` lines 180-183
builds a `text` query string and calls
`window.open(\`https://api.whatsapp.com/send?text=${text}\`, "_blank");` (line 182)
without specifying `noopener`/`noreferrer` or nulling `window.opener`; this means the
newly opened WhatsApp page keeps a reference to `window.opener` and can programmatically
navigate or manipulate the original DoubtDesk tab (reverse-tabnabbing) if that external
page or its scripts are compromised.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/components/DoubtCard.tsx
**Line:** 182:182
**Comment:**
*Security: Opening an external URL with `window.open(..., "_blank")` without `noopener`/`noreferrer` lets the new tab access `window.opener`, which enables reverse-tabnabbing attacks. Open the window with `noopener,noreferrer` (or equivalent safe link behavior) to prevent opener access.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| const handleTelegramShare = () => { | ||
| const url = encodeURIComponent(getShareUrl()); | ||
| const text = encodeURIComponent(`Check out this doubt on DoubtDesk:`); | ||
| window.open(`https://t.me/share/url?url=${url}&text=${text}`, "_blank"); |
There was a problem hiding this comment.
Suggestion: This _blank popup also omits noopener/noreferrer, so the destination page can control the opener tab and potentially redirect it. Use a safe open pattern that disables opener access. [security]
Severity Level: Major ⚠️
- ❌ Malicious Telegram share page can hijack origin tab.
- ⚠️ Doubt sharing flow exposes reverse-tabnabbing risk.Steps of Reproduction ✅
1. Navigate to any page that renders `DoubtCard`, such as the doubt permalink page
implemented in `src/app/doubts/[id]/DoubtPermalinkClient.tsx` lines 10-17, where a single
`<DoubtCard>` is rendered with `doubt={doubt}`.
2. On the doubt card, click the share icon button (footer share button in
`src/components/DoubtCard.tsx` lines 66-73) to open the share dropdown.
3. Click the "Share on Telegram" menu item defined in `src/components/DoubtCard.tsx` lines
85-88, which is wired with `onClick={handleTelegramShare}`.
4. The `handleTelegramShare` function in `src/components/DoubtCard.tsx` lines 185-188
builds `url` and `text` and then calls
`window.open(\`https://t.me/share/url?url=${url}&text=${text}\`, "_blank");` (line 188)
without a `noopener,noreferrer` window feature or clearing `window.opener`, so the opened
Telegram share page retains access to the originating DoubtDesk tab and can perform
reverse-tabnabbing by changing `window.opener.location` if that external page or embedded
scripts are compromised.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/components/DoubtCard.tsx
**Line:** 188:188
**Comment:**
*Security: This `_blank` popup also omits `noopener`/`noreferrer`, so the destination page can control the opener tab and potentially redirect it. Use a safe open pattern that disables opener access.
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
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 177: The share UI strings (e.g., "Link copied!" used in toast.success and
the hardcoded share labels/messages in the JSX) must be moved into constants to
avoid inline UI copy; add descriptive constants such as SHARE_COPY_SUCCESS and
SHARE_LABEL_* at the top of the DoubtCard.tsx (or import from a shared constants
file) and replace all hardcoded occurrences in the DoubtCard component (handlers
like the copy/share function and the JSX render blocks around lines referenced)
with those constants so the toast.success call and the share button/label text
read from the new constants.
- Around line 182-183: The external-share calls in the DoubtCard component (the
handlers that call window.open(..., "_blank"), e.g., the WhatsApp/Twitter share
handlers) leave window.opener exposed; replace those window.open calls with
creating a temporary anchor element, set its href to the share URL, set
target="_blank" and rel="noopener noreferrer", append/click/remove it (or
alternatively capture the return value from window.open and set newWindow.opener
= null and ensure referrer is suppressed), so that shared windows include
noopener and noreferrer protections.
- Around line 175-178: Make handleShare async and await
navigator.clipboard.writeText(getShareUrl()) inside a try/catch: on success call
toast.success("Link copied!"), on failure call toast.error(...) with the caught
error message and implement a simple fallback (e.g., select/copy from a hidden
input or notify the user to copy manually). Update references to handleShare
where used if necessary and ensure getShareUrl and toast are still referenced
unchanged.
🪄 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: a8cd8a75-6caa-4078-8eca-8f35b8f8d3f0
📒 Files selected for processing (1)
src/components/DoubtCard.tsx
|
hi @knoxiboy, i have made pr for this issue. Please review it and merge whenever possible. Thankyou!! |
|
Hi! This PR currently has merge conflicts with the main branch. Please rebase/merge main and resolve conflicts so we can review and merge it. Thank you! |
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
the
size:M
github-actions
Bot
removed
the
size:M
|
CodeAnt AI Incremental review completed. |
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 `@src/app/api/doubts/route.ts`:
- Around line 155-156: Duplicate declaration of replyCountSql causes a compile
error; remove the obsolete declaration. Delete the older line that redeclares
const replyCountSql using count(*)::int (the second declaration) so only one
replyCountSql remains (the version using coalesce((SELECT count(*) FROM
${repliesTable} WHERE ${repliesTable.doubtId} = ${doubtsTable.id}),
0).mapWith(Number)). Ensure no other references rely on the removed variant.
- Around line 13-15: The file has a duplicate import from "drizzle-orm" causing
duplicate identifier errors; remove the redundant import line that re-imports
and, eq, inArray, isNull, or, not, sql, SQL, ilike, desc, getTableColumns (the
older import) so only the consolidated import that includes count remains;
update the import block near the top of src/app/api/doubts/route.ts to keep the
single import containing count and drop the duplicate.
🪄 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: 006be762-95af-40e9-a1aa-c6f581c315a7
📒 Files selected for processing (3)
src/app/api/doubts/route.tssrc/app/api/replies/route.tssrc/components/DoubtCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/api/replies/route.ts
- src/components/DoubtCard.tsx
|
Hi @bhumishah2411! The CI checks are currently failing due to a compilation syntax error in \src/app/api/replies/route.ts\ where the \membership\ variable is declared twice (around lines 61 and 74). Specifically, you have: Please resolve this duplicate declaration to fix the build and test suite. Thank you! |
bhumishah2411
force-pushed
the
DoubtShareButton
branch
from
5696496 to
939ef38
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
the
size:M
github-actions
Bot
removed
the
size:M
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/api/replies/route.ts (1)
153-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce teacher authorization before using
doubt.classroomIdin POST.Line 154 performs a membership query with
doubt.classroomId!before validating it, while Lines 164-168 only enforce teacher-role checks whendoubt.classroomIdis truthy. This leaves a path where teacher-doubt replies can bypass role enforcement.🔒 Suggested fix
if (doubt.type === "teacher") { - const [teacherMembership] = await db - .select() - .from(membershipsTable) - .where( - and( - eq(membershipsTable.userEmail, email), - eq(membershipsTable.classroomId, doubt.classroomId!) - ) - ); - - if (doubt.classroomId) { - if (!teacherMembership || !canTeach(teacherMembership.role)) { - return errorResponse("Insufficient permissions to reply to this doubt", 403); - } - } + if (!doubt.classroomId) { + return errorResponse("Insufficient permissions to reply to this doubt", 403); + } + + const [teacherMembership] = await db + .select() + .from(membershipsTable) + .where( + and( + eq(membershipsTable.userEmail, email), + eq(membershipsTable.classroomId, doubt.classroomId) + ) + ); + + if (!teacherMembership || !canTeach(teacherMembership.role)) { + return errorResponse("Insufficient permissions to reply to this doubt", 403); + } }🤖 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 153 - 166, The handler queries membershipsTable using doubt.classroomId! before ensuring classroomId exists and authorizing the user; change the flow in the replies route so that inside the doubt.type === "teacher" branch you first validate doubt.classroomId (return an error if missing), then perform the DB select for teacherMembership and enforce canTeach(teacherMembership.role) (return 403 if not allowed), and remove the non-null assertion by using the validated doubt.classroomId when calling the query; use the existing symbols doubt, teacherMembership, membershipsTable, canTeach, and errorResponse to locate and update the logic.
🧹 Nitpick comments (1)
src/app/api/replies/route.ts (1)
61-73: ⚡ Quick winAvoid the duplicate membership query in GET teacher gating.
Line 63 re-queries
membershipsTablefor the same(email, classroomId)pair already queried at Line 50, adding an unnecessary DB round-trip on teacher-doubt fetches.♻️ Suggested refactor
- if (doubt.classroomId && email) { - const [membership] = await db.select().from(membershipsTable).where( + let membership; + if (doubt.classroomId && email) { + const [resolvedMembership] = await db.select().from(membershipsTable).where( and(eq(membershipsTable.userEmail, email), eq(membershipsTable.classroomId, doubt.classroomId)) ); - if (!membership) { + membership = resolvedMembership; + if (!membership) { return errorResponse("Access denied to this classroom's doubt replies", 403); } } else if (doubt.classroomId && !email) { return errorResponse("Access denied to this classroom's doubt replies", 403); } if (doubt.type === 'teacher') { - let membership; - if (email && doubt.classroomId) { - const res = await db - .select() - .from(membershipsTable) - .where( - and( - eq(membershipsTable.userEmail, email), - eq(membershipsTable.classroomId, doubt.classroomId) - ) - ); - membership = res[0]; - } - const isTeacher = membership ? canTeach(membership.role) : false; const isOwner = email ? doubt.userEmail === email : false; if (!isTeacher && !isOwner) { return errorResponse("Access denied", 403); } }🤖 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 61 - 73, Duplicate DB lookup: remove the second membershipsTable query and reuse the already-fetched membership for the same (email, doubt.classroomId) pair; specifically, in the GET teacher gating code that currently runs db.select().from(membershipsTable).where(and(eq(membershipsTable.userEmail, email), eq(membershipsTable.classroomId, doubt.classroomId))) assign or reference the previously obtained membership variable/result instead of re-querying; ensure the logic still handles the case where the first query returned no rows (membership may be undefined) and remove the redundant db.select() call to eliminate the extra round-trip.
🤖 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.
Outside diff comments:
In `@src/app/api/replies/route.ts`:
- Around line 153-166: The handler queries membershipsTable using
doubt.classroomId! before ensuring classroomId exists and authorizing the user;
change the flow in the replies route so that inside the doubt.type === "teacher"
branch you first validate doubt.classroomId (return an error if missing), then
perform the DB select for teacherMembership and enforce
canTeach(teacherMembership.role) (return 403 if not allowed), and remove the
non-null assertion by using the validated doubt.classroomId when calling the
query; use the existing symbols doubt, teacherMembership, membershipsTable,
canTeach, and errorResponse to locate and update the logic.
---
Nitpick comments:
In `@src/app/api/replies/route.ts`:
- Around line 61-73: Duplicate DB lookup: remove the second membershipsTable
query and reuse the already-fetched membership for the same (email,
doubt.classroomId) pair; specifically, in the GET teacher gating code that
currently runs
db.select().from(membershipsTable).where(and(eq(membershipsTable.userEmail,
email), eq(membershipsTable.classroomId, doubt.classroomId))) assign or
reference the previously obtained membership variable/result instead of
re-querying; ensure the logic still handles the case where the first query
returned no rows (membership may be undefined) and remove the redundant
db.select() call to eliminate the extra round-trip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89e2e226-7480-4f1b-ad4b-bf6ed623264f
📒 Files selected for processing (3)
src/app/api/doubts/route.tssrc/app/api/replies/route.tssrc/components/DoubtCard.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/api/doubts/route.ts
- src/components/DoubtCard.tsx
bhumishah2411
force-pushed
the
DoubtShareButton
branch
from
939ef38 to
e794ce1
Compare
|
Hi @bhumishah2411! Thanks for submitting this PR. Here is a detailed review of your changes: 🤖 Bot Review Feedback to ResolveCodeRabbit and CodeAnt have highlighted some important issues that need to be addressed. Please prioritize fixing these:
Do you think removing the trigger's default outline ( [Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0ec49845d95a45...
Severity Level: Major
|
|
knoxiboy
added
level:advanced
2 participants
User description
Description
Added a Share Menu to doubt cards that allows users to quickly share doubts through WhatsApp, Telegram, or by copying a direct link.
Features Added
This improves collaboration and makes it easier for students to share doubts with classmates, friends, and study groups.
Related Issue
Closes #610
Type of Change
Screenshots (if UI change)
How Has This Been Tested?
npm run devManual Testing
Checklist
npm run dev)anytypes)mainSummary by CodeRabbit
New Features
Bug Fixes
Chores
CodeAnt-AI Description
Add sharing options for doubts and fix reply access checks
What Changed
Impact
✅ Easier sharing of doubts✅ Faster sharing to WhatsApp and Telegram✅ Fewer access errors for classroom replies💡 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.