feat: add canonical /doubts/[id] permalink page and sharing #447 - #597
Merged
Conversation
|
CodeAnt AI is reviewing your PR. |
|
@angelina-2206 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. |
|
@coderabbitai review |
Contributor
WalkthroughThis PR implements a shareable doubt detail page at ChangesDoubt Permalink Feature
Supporting Changes and Refactors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
|
|
||
| afterEach(() => { | ||
| process.env.NODE_ENV = originalNodeEnv; | ||
| (process.env as any).NODE_ENV = originalNodeEnv; |
There was a problem hiding this comment.
Suggestion: Restoring NODE_ENV by direct assignment can corrupt the original state when it was unset: assigning undefined to process.env stores the literal string "undefined" instead of removing the variable. That leaks environment state across tests and can change behavior in code that checks for env presence. Restore by deleting the key when originalNodeEnv is undefined, otherwise assign the original string value. [incorrect variable usage]
Severity Level: Major ⚠️
- ⚠️ Test suite may leak mutated NODE_ENV between files.
- ⚠️ Future env-presence checks see corrupted NODE_ENV state.
- ⚠️ Notification seed route behavior may differ across runs.Steps of Reproduction ✅
1. Run the Jest test suite so that `src/__tests__/api/notifications-test-seed.test.ts` is
executed; on file load, `const originalNodeEnv = process.env.NODE_ENV;` at line 32
captures the current `NODE_ENV` value (which will be `undefined` if the environment didn't
set it).
2. Within the same run, execute the test `"hides the seed route from GET requests in
production"` defined at lines 56–66, which sets `(process.env as any).NODE_ENV =
'production';` at line 57 and then calls `GET()` imported from
`src/app/api/notifications/test-seed/route.ts` (required at line 28).
3. After the test finishes, Jest invokes the `afterEach` hook at lines 41–43, executing
`(process.env as any).NODE_ENV = originalNodeEnv;` at line 42; when `originalNodeEnv` is
`undefined`, Node.js coerces this to the string `"undefined"`, leaving
`process.env.NODE_ENV` defined instead of removing it.
4. Subsequent code in the same Jest worker, such as the `canSeedNotifications` helper in
`src/app/api/notifications/test-seed/route.ts:6` or any other module that inspects
environment variables, now observes `process.env.NODE_ENV === "undefined"` and
`("NODE_ENV" in process.env) === true`, meaning the process environment has been mutated
away from its original "unset" state and this mutated value can leak across tests.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/__tests__/api/notifications-test-seed.test.ts
**Line:** 42:42
**Comment:**
*Incorrect Variable Usage: Restoring `NODE_ENV` by direct assignment can corrupt the original state when it was unset: assigning `undefined` to `process.env` stores the literal string `"undefined"` instead of removing the variable. That leaks environment state across tests and can change behavior in code that checks for env presence. Restore by deleting the key when `originalNodeEnv` is `undefined`, otherwise assign the original string value.
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
+12
to
+17
| try { | ||
| const { id } = await params; | ||
| const doubtId = parseInt(id, 10); | ||
|
|
||
| if (isNaN(doubtId)) { | ||
| return NextResponse.json({ error: "Invalid doubt ID" }, { status: 400 }); |
There was a problem hiding this comment.
Suggestion: ID parsing is too permissive: parseInt accepts mixed strings like "123abc" and treats them as valid IDs, which can route malformed input to the wrong record. Validate the entire segment as a strict positive integer before querying. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Malformed IDs can resolve to unintended doubt records.
- ⚠️ Logs or analytics may show misleading requested IDs.Steps of Reproduction ✅
1. Ensure any doubt exists with a known numeric ID, for example by creating one via `POST
/api/doubts` in `src/app/api/doubts/route.ts:229-136`, which inserts into `doubtsTable`
and returns `newDoubt.id`.
2. From any client, issue `GET /api/doubts/123xyz` where `123` is the numeric ID of an
existing doubt and the trailing `xyz` simulates a malformed or tampered route segment.
3. In `src/app/api/doubts/[id]/route.ts`, the handler reads `{ id }` from params at line
12; `id` is the literal string `"123xyz"`. At line 13 it computes `const doubtId =
parseInt(id, 10);`, which returns `123` even though the path includes extra characters.
4. The NaN guard at lines 15-17 (`if (isNaN(doubtId)) { ... }`) does not trigger, and the
query at lines 23-30 uses `eq(doubtsTable.id, doubtId)` with `doubtId = 123`, returning
and exposing the doubt whose true ID is `123` for a request URL that does not strictly
match that ID.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/[id]/route.ts
**Line:** 12:17
**Comment:**
*Incorrect Condition Logic: ID parsing is too permissive: `parseInt` accepts mixed strings like `"123abc"` and treats them as valid IDs, which can route malformed input to the wrong record. Validate the entire segment as a strict positive integer before querying.
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
+36
to
+51
| // Classroom membership guard | ||
| if (doubt.classroomId) { | ||
| if (!email) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
| const membership = await requireMembership(email, doubt.classroomId); | ||
|
|
||
| // Teacher doubts visibility guard | ||
| if (doubt.type === "teacher") { | ||
| const isTeacher = ["teacher", "owner", "admin"].includes(membership.role.toLowerCase()); | ||
| const isAuthor = doubt.userEmail === email; | ||
| if (!isTeacher && !isAuthor) { | ||
| return NextResponse.json({ error: "Access denied to this doubt" }, { status: 403 }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The teacher-only visibility check is only executed inside the classroom guard, so a type === "teacher" doubt with no classroomId is returned to anyone who knows the permalink. Apply the teacher-visibility rule independently of classroom membership so direct links cannot bypass access control. [security]
Severity Level: Critical 🚨
- ❌ Teacher-only doubts exposed via direct /api/doubts/[id] calls.
- ⚠️ Permalink API weaker than list endpoint authorization.
- ⚠️ Notifications link to /doubts/{id} exposing restricted threads.Steps of Reproduction ✅
1. Create a teacher-only doubt without a classroom by calling `POST /api/doubts`
implemented in `src/app/api/doubts/route.ts:229-136`, sending a JSON body with `type:
"teacher"` and `classroomId` omitted or null. This is accepted because `createDoubtSchema`
in `src/lib/validations/doubt.ts:4-15` allows `type: "teacher"` with a nullable
`classroomId`.
2. As an unauthenticated or different user (no Clerk session), call the list endpoint `GET
/api/doubts` (handled in `src/app/api/doubts/route.ts:25-227`) without `classroomId` and
observe the teacher doubt is filtered out: when `classroomId` is absent,
`conditions.push(isNull(doubtsTable.classroomId))` at lines 53-57 is applied, and because
`isTeacher` remains false, `teacherCondition` at lines 82-87 adds
`not(eq(doubtsTable.type, "teacher"))`, hiding teacher-type doubts from the listing for
non-authors.
3. Still as that unauthenticated/different user, call the permalink detail endpoint `GET
/api/doubts/{id}` (Next.js maps this to `src/app/api/doubts/[id]/route.ts:8-111`). The
handler loads the doubt by ID at lines 23-30 without any prior auth and obtains `doubt`
with `classroomId === null`.
4. Because `doubt.classroomId` is falsy, the classroom membership guard block at lines
36-51 (`if (doubt.classroomId) { ... }`) is entirely skipped, including the
teacher-visibility guard at lines 43-50. Execution falls through to the final
`NextResponse.json({ ...doubt, tags, hasLiked, hasBookmarked })` at lines 100-105,
returning the full teacher-only doubt to any caller who knows or guesses the numeric ID,
bypassing the teacher-only filtering enforced in the list API.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/[id]/route.ts
**Line:** 36:51
**Comment:**
*Security: The teacher-only visibility check is only executed inside the classroom guard, so a `type === "teacher"` doubt with no `classroomId` is returned to anyone who knows the permalink. Apply the teacher-visibility rule independently of classroom membership so direct links cannot bypass access control.
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
+21
to
+44
| const [doubt] = await db | ||
| .select({ | ||
| subject: doubtsTable.subject, | ||
| content: doubtsTable.content, | ||
| }) | ||
| .from(doubtsTable) | ||
| .where(eq(doubtsTable.id, doubtId)) | ||
| .limit(1); | ||
|
|
||
| if (!doubt) { | ||
| return { | ||
| title: "Doubt Not Found", | ||
| }; | ||
| } | ||
|
|
||
| const title = doubt.subject || "Doubt Thread"; | ||
| const contentSnippet = doubt.content | ||
| ? doubt.content.slice(0, 150) + (doubt.content.length > 150 ? "..." : "") | ||
| : "View this doubt on DoubtDesk."; | ||
|
|
||
| return { | ||
| title, | ||
| description: contentSnippet, | ||
| }; |
There was a problem hiding this comment.
Suggestion: generateMetadata reads and exposes subject/content before any auth or membership checks, so protected doubts can leak text in metadata/preview contexts. Reuse the same visibility checks used by the page body and return generic metadata when unauthorized. [security]
Severity Level: Critical 🚨
- ❌ Classroom doubt text leaked via route metadata.
- ❌ Teacher-only doubts exposed in link previews.
- ⚠️ Unauthorized crawlers can index sensitive doubt snippets.Steps of Reproduction ✅
1. Create a classroom-scoped doubt by calling `POST /api/doubts` in
`src/app/api/doubts/route.ts:229-136` with a non-null `classroomId`. `createDoubtSchema`
in `src/lib/validations/doubt.ts:4-15` permits this, and membership checks at lines 20-37
of the POST handler ensure only classroom members can create it.
2. From a user who is not a member of that classroom (no row in `membershipsTable` for
their email/classroomId), request `/doubts/{id}` in a browser or via an HTTP client. The
page component `DoubtPermalinkPage` in `src/app/doubts/[id]/page.tsx:52-127` loads the
doubt at lines 65-71, detects `doubt.classroomId` truthy at line 77, and then checks
membership/ownership at lines 82-108. With no membership and no owned classroom, `role`
remains null and the user is redirected to `/403` at lines 110-112, so the body correctly
denies access.
3. However, Next.js invokes `generateMetadata` in the same file
(`src/app/doubts/[id]/page.tsx:9-50`) for this route regardless of user membership. Inside
`generateMetadata`, the function parses the ID at lines 12-18 and queries `doubtsTable`
for `{ subject, content }` at lines 21-28 without consulting `currentUser`,
`membershipsTable`, or any teacher/classroom guard.
4. When the doubt exists, `generateMetadata` builds a `title` and a `contentSnippet` from
`doubt.subject` and `doubt.content` at lines 36-40 and returns them at lines 41-44. This
causes the HTML `<title>` and `<meta name="description">` for `/doubts/{id}` to contain
the protected subject and the first 150 characters of content even for unauthorized
visitors, leaking classroom- or teacher-only content via metadata and link previews while
the main page still redirects them to `/403`.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/doubts/[id]/page.tsx
**Line:** 21:44
**Comment:**
*Security: `generateMetadata` reads and exposes `subject`/`content` before any auth or membership checks, so protected doubts can leak text in metadata/preview contexts. Reuse the same visibility checks used by the page body and return generic metadata when unauthorized.
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
+76
to
+122
| // Classroom membership check | ||
| if (doubt.classroomId) { | ||
| if (!email) { | ||
| redirect("/sign-in"); | ||
| } | ||
|
|
||
| // Check membership | ||
| const [membership] = await db | ||
| .select({ role: membershipsTable.role }) | ||
| .from(membershipsTable) | ||
| .where( | ||
| and( | ||
| eq(membershipsTable.userEmail, email), | ||
| eq(membershipsTable.classroomId, doubt.classroomId) | ||
| ) | ||
| ); | ||
|
|
||
| let role = membership?.role ?? null; | ||
| if (!role) { | ||
| // Check if teacher owns the classroom | ||
| const [ownedClassroom] = await db | ||
| .select() | ||
| .from(classroomsTable) | ||
| .where( | ||
| and( | ||
| eq(classroomsTable.id, doubt.classroomId), | ||
| eq(classroomsTable.teacherEmail, email) | ||
| ) | ||
| ); | ||
| if (ownedClassroom) { | ||
| role = "owner"; | ||
| } | ||
| } | ||
|
|
||
| if (!role) { | ||
| redirect("/403"); | ||
| } | ||
|
|
||
| // Teacher doubts visibility rules | ||
| if (doubt.type === "teacher") { | ||
| const isTeacher = ["teacher", "owner", "admin"].includes(role.toLowerCase()); | ||
| const isAuthor = doubt.userEmail === email; | ||
| if (!isTeacher && !isAuthor) { | ||
| redirect("/403"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The page enforces teacher-doubt restrictions only when classroomId exists, so a non-classroom teacher doubt can be viewed directly via /doubts/[id] even though list endpoints hide these entries. Move teacher visibility checks outside the classroom-only branch to keep authorization consistent. [security]
Severity Level: Critical 🚨
- ❌ Teacher-only community doubts viewable via /doubts/{id}.
- ⚠️ Permalink page weaker than /api/doubts list authorization.
- ⚠️ Notification links may expose teacher-only threads to others.Steps of Reproduction ✅
1. Create a teacher-only community doubt (no classroom) using `POST /api/doubts` in
`src/app/api/doubts/route.ts:229-136` with payload `{ type: "teacher", classroomId: null,
... }`. This is allowed because `createDoubtSchema` in `src/lib/validations/doubt.ts:4-15`
does not require `classroomId` when `type` is `"teacher"`.
2. As a different user or an unauthenticated visitor, navigate in a browser to
`/doubts/{id}` where `{id}` is the ID of the doubt created in step 1. Next.js routes this
to `DoubtPermalinkPage` in `src/app/doubts/[id]/page.tsx:52-127`.
3. `DoubtPermalinkPage` parses the ID and loads the doubt at lines 55-71
(`db.select().from(doubtsTable).where(eq(doubtsTable.id, doubtId))`) without any prior
check on the current user; for a non-classroom doubt, `doubt.classroomId` is `null`.
4. The classroom membership and teacher visibility logic at lines 76-121 is guarded by `if
(doubt.classroomId) { ... }`. Because `doubt.classroomId` is falsy, the entire
block—including the teacher-only check at lines 114-121 which restricts `doubt.type ===
"teacher"` to teachers/owners/admins or the author—is skipped, and the function proceeds
to render `<DoubtPermalinkClient initialDoubt={doubt as any} />` at lines 124-126,
allowing any visitor with the permalink to view teacher-only non-classroom doubts that are
otherwise hidden from the list API.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/doubts/[id]/page.tsx
**Line:** 76:122
**Comment:**
*Security: The page enforces teacher-doubt restrictions only when `classroomId` exists, so a non-classroom teacher doubt can be viewed directly via `/doubts/[id]` even though list endpoints hide these entries. Move teacher visibility checks outside the classroom-only branch to keep authorization consistent.
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
| import { ArrowLeft } from "lucide-react"; | ||
| import DoubtCard from "@/components/DoubtCard"; | ||
| import DoubtRepliesModal from "@/components/DoubtRepliesModal"; | ||
| import type { Doubt } from "@/types"; |
There was a problem hiding this comment.
Suggestion: The Doubt type import is unused, which adds dead code noise and can fail stricter lint/TS settings in CI. Remove unused imports to keep build checks stable. [code quality]
Severity Level: Major ⚠️
- ⚠️ ESLint reports unused type import in permalink client.
- ⚠️ Consumes warning budget in PR code-quality workflow.Steps of Reproduction ✅
1. Open `src/app/doubts/[id]/DoubtPermalinkClient.tsx` and observe the import `import type
{ Doubt } from "@/types";` at line 8, while the rest of the file (lines 10-89) uses `any`
for `initialDoubt` and `useState<any>` and never references `Doubt`.
2. Note that ESLint is configured and run via the `lint` script in `package.json:5-9`
(`"lint": "next lint"`) and the `lint` job in
`.github/workflows/pr-code-quality.yml:37-55`, which runs `npx eslint . --ext .ts,.tsx
--max-warnings=20`.
3. Running `npm run lint` locally or in CI will apply `eslint-config-next`'s recommended
rules (including unused variable/import checks), causing ESLint to report the `Doubt`
import in `DoubtPermalinkClient.tsx:8` as unused.
4. This unused import generates avoidable lint warnings and consumes part of the
`--max-warnings=20` budget in the PR code-quality workflow, increasing noise and the
likelihood that additional issues push the job over the warning threshold.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/doubts/[id]/DoubtPermalinkClient.tsx
**Line:** 8:8
**Comment:**
*Code Quality: The `Doubt` type import is unused, which adds dead code noise and can fail stricter lint/TS settings in CI. Remove unused imports to keep build checks stable.
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
+23
to
+35
| const fetchDoubt = async () => { | ||
| try { | ||
| const userName = localStorage.getItem("anonymous_user"); | ||
| const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : ""); | ||
| const res = await fetch(url); | ||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| setDoubt(data); | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to fetch doubt details:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Suggestion: Concurrent calls to fetchDoubt can resolve out of order and overwrite newer state with stale data, because every successful response immediately calls setDoubt(data) with no request ordering or cancellation. This can happen when useEffect fetch and action-triggered refreshes overlap. Track request sequence (or use AbortController) so only the latest response updates state. [race condition]
Severity Level: Major ⚠️
- ⚠️ Permalink doubt page may display outdated like/bookmark status.
- ⚠️ Reply count on permalink can regress after rapid actions.Steps of Reproduction ✅
1. Navigate to `/doubts/123` which renders `DoubtPermalinkPage` in
`src/app/doubts/[id]/page.tsx:52-56`, and returns `<DoubtPermalinkClient
initialDoubt={doubt as any} />` at `page.tsx:124-125`.
2. On mount, `DoubtPermalinkClient` in
`src/app/doubts/[id]/DoubtPermalinkClient.tsx:14-21` initializes state and `useEffect` at
lines 37-39 calls `fetchDoubt()` (lines 23-35), issuing an initial GET `/api/doubts/:id`
request.
3. While that request is in flight, rapidly perform two actions on the doubt (e.g., like
then bookmark) via the UI rendered by `DoubtCard`
(`src/components/DoubtCard.tsx:178-246`), which triggers `handleAction` / `handleBookmark`
(lines 71-107 and 152-170); on success these call `onUpdate()` (lines 89, 141, 161), which
is wired to `fetchDoubt` via `onUpdate={fetchDoubt}` at `DoubtPermalinkClient.tsx:71-74`.
4. This sequence results in multiple concurrent `fetchDoubt` calls (initial `useEffect`
plus one or more `onUpdate` calls) all hitting `/api/doubts/:id`
(`DoubtPermalinkClient.tsx:23-31`); because each successful response calls
`setDoubt(data)` unconditionally (line 30), a slower, earlier request can resolve last and
overwrite the newer state from a later request, leaving the permalink UI showing stale
like/bookmark/replyCount information.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/doubts/[id]/DoubtPermalinkClient.tsx
**Line:** 23:35
**Comment:**
*Race Condition: Concurrent calls to `fetchDoubt` can resolve out of order and overwrite newer state with stale data, because every successful response immediately calls `setDoubt(data)` with no request ordering or cancellation. This can happen when `useEffect` fetch and action-triggered refreshes overlap. Track request sequence (or use `AbortController`) so only the latest response updates state.
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/gssoc-auto-labeler.yml (1)
19-19:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin action to commit SHA instead of mutable tag.
The action is currently pinned to
@v6, a mutable tag reference that creates a supply chain security risk. Tags can be moved or compromised, allowing malicious code execution in the workflow.Pin to a specific commit SHA with a comment indicating the version for maintainability.
🔒 Proposed fix to pin to commit SHA
- - name: Intelligent Auto-Labeling - uses: actions/github-script@v6 + - name: Intelligent Auto-Labeling + uses: actions/github-script@d7906e4ad0b1822421a7e6a35d5ca353c962f410 # v6.4.1Note: Verify the commit SHA corresponds to the desired v6 release from the actions/github-script releases page.
As per coding guidelines:
.github/workflows/**should use pinned action versions.🤖 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 @.github/workflows/gssoc-auto-labeler.yml at line 19, Replace the mutable reference "actions/github-script@v6" with a pinned commit SHA for that action (e.g., "actions/github-script@<commit-sha>") and add an inline comment noting the v6 release it corresponds to; update the uses entry that currently reads actions/github-script@v6 to use the specific commit SHA so the workflow is immutable and include a short comment like "pins v6 release" for maintainability.
🧹 Nitpick comments (8)
src/app/doubts/[id]/page.tsx (1)
83-104: 💤 Low valueAdd
.limit(1)to membership and classroom queries for consistency.Both queries expect a single row (membership has a unique constraint, classroom checks by primary key), but adding
.limit(1)would be consistent with the doubt query on line 70 and potentially allow the database to short-circuit scanning.📋 Suggested changes
const [membership] = await db .select({ role: membershipsTable.role }) .from(membershipsTable) .where( and( eq(membershipsTable.userEmail, email), eq(membershipsTable.classroomId, doubt.classroomId) ) - ); + ) + .limit(1); let role = membership?.role ?? null; if (!role) { // Check if teacher owns the classroom const [ownedClassroom] = await db .select() .from(classroomsTable) .where( and( eq(classroomsTable.id, doubt.classroomId), eq(classroomsTable.teacherEmail, email) ) - ); + ) + .limit(1);🤖 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/doubts/`[id]/page.tsx around lines 83 - 104, The membership and ownedClassroom queries (the db select against membershipsTable and classroomsTable used to derive role and ownedClassroom) should include .limit(1) so they return at most one row like the earlier doubt query; update the query that reads membership (select({ role: membershipsTable.role }).from(membershipsTable).where(...)) and the query that reads ownedClassroom (select().from(classroomsTable).where(...)) to append .limit(1) to each to ensure consistent single-row semantics and allow the DB to short-circuit.src/components/DoubtCard.tsx (2)
312-319: ⚡ Quick winExtract hardcoded strings to constants.
Lines 315-316 contain hardcoded strings "Share doubt" in both
titleandaria-label. As per coding guidelines for**/*.tsxfiles, hardcoded strings should be extracted to constants.📝 Example refactor
+const SHARE_BUTTON_LABEL = "Share doubt"; + <button onClick={handleShare} 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" - title="Share doubt" - aria-label="Share doubt" + title={SHARE_BUTTON_LABEL} + aria-label={SHARE_BUTTON_LABEL} >🤖 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 312 - 319, Extract the hardcoded "Share doubt" strings used in the button's title and aria-label into constants and reference them from the DoubtCard component; add a constant like SHARE_DOUBT_LABEL (or similar) near the top of the file (above the DoubtCard function) and replace both title and aria-label values with that constant, ensuring both attributes use the same exported/local constant and updating any relevant tests or translations if applicable; keep the onClick handler handleShare and the button markup unchanged except for replacing the literal strings with the constant.
175-175: ⚡ Quick winExtract hardcoded string to a constant.
The success message "Link copied!" is hardcoded. As per coding guidelines for
**/*.tsxfiles, hardcoded strings should be extracted to constants.📝 Example refactor
+const SHARE_SUCCESS_MESSAGE = "Link copied!"; + export default function DoubtCard({ ... }) { ... const handleShare = () => { const url = `${window.location.origin}/doubts/${doubt.id}`; navigator.clipboard.writeText(url); - toast.success("Link copied!"); + toast.success(SHARE_SUCCESS_MESSAGE); };🤖 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 175, The toast.success call in DoubtCard.tsx uses a hardcoded string ("Link copied!"); extract that string into a named constant (e.g., COPY_SUCCESS_MESSAGE or LINK_COPIED_MESSAGE) and replace the literal in the toast.success(...) call inside the DoubtCard component (the handler that triggers the copy toast). Define the constant at the top of the file (or import from a shared constants module) so it can be reused and follows the project's constant naming conventions.src/app/doubts/[id]/DoubtPermalinkClient.tsx (3)
10-12: ⚡ Quick winReplace
anywith properDoubttype.The
Doubttype is imported from@/typesbut the prop usesany, which defeats TypeScript's type safety.🔧 Proposed fix
interface DoubtPermalinkClientProps { - initialDoubt: any; + initialDoubt: Doubt; }🤖 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/doubts/`[id]/DoubtPermalinkClient.tsx around lines 10 - 12, Change the prop type for initialDoubt from any to the proper Doubt type: update the DoubtPermalinkClientProps interface to use initialDoubt: Doubt, ensure the Doubt type is imported from "`@/types`" at the top of DoubtPermalinkClient.tsx (and remove any unused any usage), and update usages of initialDoubt in the DoubtPermalinkClient component to satisfy the stricter type if any adjustments are needed.
70-86: ⚡ Quick winConsider adding a loading state during
fetchDoubt.While
initialDoubtprovides baseline data, the component fetches updated details (tags, hasLiked, hasBookmarked) on mount without showing a loading indicator. This can cause a brief flash when the data updates. As per coding guidelines for TSX files, missing loading states should be addressed.💡 Example implementation
export default function DoubtPermalinkClient({ initialDoubt }: DoubtPermalinkClientProps) { + const [isLoading, setIsLoading] = useState(false); const [doubt, setDoubt] = useState<any>({ ...initialDoubt, tags: [], replyCount: initialDoubt.replyCount || 0, hasLiked: false, hasBookmarked: false, }); const fetchDoubt = async () => { + setIsLoading(true); try { const userName = localStorage.getItem("anonymous_user"); const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : ""); const res = await fetch(url); if (res.ok) { const data = await res.json(); setDoubt(data); } } catch (error) { console.error("Failed to fetch doubt details:", error); + } finally { + setIsLoading(false); } };Then show a skeleton or spinner in the UI when
isLoadingis true.🤖 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/doubts/`[id]/DoubtPermalinkClient.tsx around lines 70 - 86, Add a loading state around the fetchDoubt flow: introduce a boolean state (e.g., isLoading) that is set true before calling fetchDoubt and false after it resolves/fails, then use that state to render a skeleton/spinner instead of the actual DoubtCard and DoubtRepliesModal while loading; specifically update the component that currently uses initialDoubt and calls fetchDoubt on mount (referencing fetchDoubt, initialDoubt, handleScrollToComments) so when isLoading is true you render a placeholder UI, otherwise render the existing DoubtCard and DoubtRepliesModal components.
51-67: ⚡ Quick winExtract hardcoded UI strings to constants.
Lines 62 and 65 contain hardcoded strings "Back to Feed" and "Doubt Permalink". As per coding guidelines for
**/*.tsxfiles, hardcoded strings should be extracted to constants for maintainability and potential i18n support.📝 Example refactor
At the top of the file or in a shared constants file:
const PERMALINK_STRINGS = { BACK_TO_FEED: "Back to Feed", TITLE: "Doubt Permalink" } as const;Then use:
- Back to Feed + {PERMALINK_STRINGS.BACK_TO_FEED} </Link> - <h1 className="text-xl font-black text-slate-900 dark:text-white tracking-tight uppercase italic"> - Doubt <span className="text-blue-500">Permalink</span> - </h1> + <h1 className="text-xl font-black text-slate-900 dark:text-white tracking-tight uppercase italic"> + {PERMALINK_STRINGS.TITLE.split(' ')[0]} <span className="text-blue-500">{PERMALINK_STRINGS.TITLE.split(' ')[1]}</span> + </h1>🤖 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/doubts/`[id]/DoubtPermalinkClient.tsx around lines 51 - 67, Extract the hardcoded UI strings "Back to Feed" and "Doubt Permalink" into a constant (e.g., PERMALINK_STRINGS) and replace their inline usage in the JSX of DoubtPermalinkClient (the Link text and the h1 title) with PERMALINK_STRINGS.BACK_TO_FEED and PERMALINK_STRINGS.TITLE; declare the constant at the top of this file (or import from a shared constants file) and type it as const to keep literals immutable and friendly for future i18n extraction.src/__tests__/api/doubts-id.test.ts (1)
45-78: ⚖️ Poor tradeoffConsider expanding test coverage for classroom and teacher-visibility scenarios.
The current tests verify the 404 case and a public doubt, but the PR objectives include several access-control requirements that aren't tested:
- Classroom doubts requiring membership (401 for unauthenticated, 403 for non-members)
- Teacher-type visibility restrictions (only teacher/owner/admin or author can view)
- Doubts with tags and reply counts
- Authenticated user interaction flags (hasLiked, hasBookmarked)
While the implementation appears correct, additional test cases would improve confidence in the access-control logic.
🤖 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-id.test.ts` around lines 45 - 78, Add unit tests in src/__tests__/api/doubts-id.test.ts that cover the missing access-control scenarios by reusing the existing helpers (GET, mockCurrentUser, selectResultQueue) and stubbing DB selects for doubtsTable/tagsTable/likes/bookmarks/replies; specifically add tests that assert: unauthenticated access to a classroom doubt returns 401, authenticated non-member returns 403 and member returns 200 with doubt details; teacher-type doubts only return 200 for the author, classroom teacher/owner/admin and 403 for others; include tests that check tags and reply count are returned and that hasLiked/hasBookmarked are true/false by pushing appropriate rows into selectResultQueue for likes/bookmarks and replies. Ensure each test sets mockCurrentUser appropriately and calls GET(new Request(...), { params: Promise.resolve({ id: '42' }) }) then asserts status and response fields.src/__tests__/api/rooms-members.test.ts (1)
113-156: ⚡ Quick winConsider adding test coverage for admin and owner roles.
The test suite currently verifies email visibility for teacher (privileged) and student (non-privileged) roles, but does not explicitly test admin and owner roles. While these roles use the same
canViewMemberEmailscode path as teacher, adding explicit test cases would improve coverage and document the intended behavior for all privileged roles.🧪 Example test case for admin role
it('includes member emails for admin requesters', async () => { currentUserMock.mockResolvedValue({ primaryEmailAddress: { emailAddress: 'admin@example.com' }, }); checkUserBlockMock.mockResolvedValue({ isBlocked: false }); selectResultQueue.push( [{ id: 1, userEmail: 'admin@example.com', role: 'admin', classroomId: 1 }], [{ count: 2 }], [ { id: 1, userEmail: 'admin@example.com', role: 'admin', joinedAt: new Date('2026-01-01T00:00:00.000Z'), }, { id: 2, userEmail: 'student@example.com', role: 'student', joinedAt: new Date('2026-01-02T00:00:00.000Z'), }, ], ); const res = (await GET(new Request('http://localhost/api/rooms/members?classroomId=1')))!; const json = await res.json(); expect(res.status).toBe(200); expect(json.members[0].userEmail).toBe('admin@example.com'); expect(json.members[1].userEmail).toBe('student@example.com'); });🤖 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/rooms-members.test.ts` around lines 113 - 156, Add explicit test cases for admin and owner roles mirroring the existing teacher test: create two new it blocks that set currentUserMock to return primaryEmailAddress email for 'admin@example.com' and 'owner@example.com', set checkUserBlockMock to { isBlocked: false }, push corresponding rows into selectResultQueue (first row with role 'admin' or 'owner', count row, and members array including a student), call GET(new Request(...)) and assert res.status === 200 and that json.members contains the requester email and the student email (use the same expectations as the teacher test). Ensure the new tests reference currentUserMock, checkUserBlockMock, selectResultQueue, and GET so they exercise the same canViewMemberEmails path.
🤖 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/`[id]/route.ts:
- Around line 64-84: The like-check trusts a caller-supplied query param
(userName) via userLikesCheck and uses it to set hasLiked; instead derive the
identity server-side or validate a signed/opaque anonymous token before using it
in the like lookup. Update the route handler to ignore unvalidated
searchParams.get("userName"), instead obtain the user identifier from the
authenticated session (or verify a signed anonymous token) and use that
validated id in the db query (replace userLikesCheck with the server-derived id
when querying likesTable for doubtId), and adjust the API contract /
DoubtPermalinkClient.tsx to send only the validated token or nothing if the
server will derive identity. Ensure the handler rejects or treats as
unauthenticated any requests with invalid/unsigned anonymous identifiers.
In `@src/app/doubts/`[id]/DoubtPermalinkClient.tsx:
- Around line 23-35: The fetchDoubt function currently only logs failures to
console; add user-facing feedback by introducing an error state or calling the
app's toast/notification helper from the catch block so users see a message when
fetching initialDoubt.id fails. Specifically, add a state (e.g., const
[fetchError, setFetchError] = useState<string | null>(null)) and in fetchDoubt's
catch call setFetchError(error.message || "Failed to load doubt") or call the
existing toast/showToast helper with that message, and clear the error when the
fetch succeeds (inside the res.ok branch call setFetchError(null)). Update the
component render to display a brief error UI (toast/banner) when fetchError is
set so users get visible feedback instead of only console.error.
- Around line 37-39: The useEffect currently calls fetchDoubt but doesn’t
include it in the dependency array, which can close over stale values; fix by
either adding fetchDoubt to the dependency array of the useEffect or memoizing
fetchDoubt with useCallback (so its identity only changes when its true
dependencies change), referencing the existing fetchDoubt function and the
effect that depends on initialDoubt.id; ensure the chosen approach updates the
dependency list to include fetchDoubt (or the stable callback) so the effect
always uses the current closure.
In `@src/app/doubts/`[id]/page.tsx:
- Around line 9-50: generateMetadata currently reads subject/content from the
database (db.select on doubtsTable) and can leak private doubt details; update
generateMetadata to perform the same access checks as the page (verify the
current user's classroom membership and teacher-type visibility rules) before
using doubt.subject or doubt.content for metadata, and if the user is not
authorized return generic metadata (e.g., title "Doubt Thread" and a
non-sensitive description) instead; locate the logic in generateMetadata, the
db.select(..).from(doubtsTable).where(eq(doubtsTable.id, doubtId)) call and
reuse or call the same authorization helper/method used by the page handler to
decide whether to expose detailed metadata.
- Around line 124-126: Remove the unnecessary "as any" cast when passing data
into DoubtPermalinkClient; change the call in page.tsx from
<DoubtPermalinkClient initialDoubt={doubt as any} /> to <DoubtPermalinkClient
initialDoubt={doubt} /> and instead make initialDoubt strongly typed by
importing or deriving a shared Doubt type (e.g., Doubt, DoubtDTO, or the type
returned by your DB/API fetch) and updating DoubtPermalinkClient's prop
signature to use that type so normalization (tags, replyCount, hasLiked,
hasBookmarked) remains correct without suppressing type safety.
In `@src/components/DoubtCard.tsx`:
- Around line 172-176: The handleShare function currently calls
navigator.clipboard.writeText(url) without handling failures; wrap the clipboard
call in a try/catch or handle its returned Promise and on success call
toast.success("Link copied!"), and on failure call toast.error with a helpful
message (include the url when appropriate) and optionally fallback to selecting
and copying via a temporary input; update handleShare to reference doubt.id and
navigator.clipboard.writeText so users see a success toast only on success and
an error toast when the writeText operation fails.
---
Outside diff comments:
In @.github/workflows/gssoc-auto-labeler.yml:
- Line 19: Replace the mutable reference "actions/github-script@v6" with a
pinned commit SHA for that action (e.g., "actions/github-script@<commit-sha>")
and add an inline comment noting the v6 release it corresponds to; update the
uses entry that currently reads actions/github-script@v6 to use the specific
commit SHA so the workflow is immutable and include a short comment like "pins
v6 release" for maintainability.
---
Nitpick comments:
In `@src/__tests__/api/doubts-id.test.ts`:
- Around line 45-78: Add unit tests in src/__tests__/api/doubts-id.test.ts that
cover the missing access-control scenarios by reusing the existing helpers (GET,
mockCurrentUser, selectResultQueue) and stubbing DB selects for
doubtsTable/tagsTable/likes/bookmarks/replies; specifically add tests that
assert: unauthenticated access to a classroom doubt returns 401, authenticated
non-member returns 403 and member returns 200 with doubt details; teacher-type
doubts only return 200 for the author, classroom teacher/owner/admin and 403 for
others; include tests that check tags and reply count are returned and that
hasLiked/hasBookmarked are true/false by pushing appropriate rows into
selectResultQueue for likes/bookmarks and replies. Ensure each test sets
mockCurrentUser appropriately and calls GET(new Request(...), { params:
Promise.resolve({ id: '42' }) }) then asserts status and response fields.
In `@src/__tests__/api/rooms-members.test.ts`:
- Around line 113-156: Add explicit test cases for admin and owner roles
mirroring the existing teacher test: create two new it blocks that set
currentUserMock to return primaryEmailAddress email for 'admin@example.com' and
'owner@example.com', set checkUserBlockMock to { isBlocked: false }, push
corresponding rows into selectResultQueue (first row with role 'admin' or
'owner', count row, and members array including a student), call GET(new
Request(...)) and assert res.status === 200 and that json.members contains the
requester email and the student email (use the same expectations as the teacher
test). Ensure the new tests reference currentUserMock, checkUserBlockMock,
selectResultQueue, and GET so they exercise the same canViewMemberEmails path.
In `@src/app/doubts/`[id]/DoubtPermalinkClient.tsx:
- Around line 10-12: Change the prop type for initialDoubt from any to the
proper Doubt type: update the DoubtPermalinkClientProps interface to use
initialDoubt: Doubt, ensure the Doubt type is imported from "`@/types`" at the top
of DoubtPermalinkClient.tsx (and remove any unused any usage), and update usages
of initialDoubt in the DoubtPermalinkClient component to satisfy the stricter
type if any adjustments are needed.
- Around line 70-86: Add a loading state around the fetchDoubt flow: introduce a
boolean state (e.g., isLoading) that is set true before calling fetchDoubt and
false after it resolves/fails, then use that state to render a skeleton/spinner
instead of the actual DoubtCard and DoubtRepliesModal while loading;
specifically update the component that currently uses initialDoubt and calls
fetchDoubt on mount (referencing fetchDoubt, initialDoubt,
handleScrollToComments) so when isLoading is true you render a placeholder UI,
otherwise render the existing DoubtCard and DoubtRepliesModal components.
- Around line 51-67: Extract the hardcoded UI strings "Back to Feed" and "Doubt
Permalink" into a constant (e.g., PERMALINK_STRINGS) and replace their inline
usage in the JSX of DoubtPermalinkClient (the Link text and the h1 title) with
PERMALINK_STRINGS.BACK_TO_FEED and PERMALINK_STRINGS.TITLE; declare the constant
at the top of this file (or import from a shared constants file) and type it as
const to keep literals immutable and friendly for future i18n extraction.
In `@src/app/doubts/`[id]/page.tsx:
- Around line 83-104: The membership and ownedClassroom queries (the db select
against membershipsTable and classroomsTable used to derive role and
ownedClassroom) should include .limit(1) so they return at most one row like the
earlier doubt query; update the query that reads membership (select({ role:
membershipsTable.role }).from(membershipsTable).where(...)) and the query that
reads ownedClassroom (select().from(classroomsTable).where(...)) to append
.limit(1) to each to ensure consistent single-row semantics and allow the DB to
short-circuit.
In `@src/components/DoubtCard.tsx`:
- Around line 312-319: Extract the hardcoded "Share doubt" strings used in the
button's title and aria-label into constants and reference them from the
DoubtCard component; add a constant like SHARE_DOUBT_LABEL (or similar) near the
top of the file (above the DoubtCard function) and replace both title and
aria-label values with that constant, ensuring both attributes use the same
exported/local constant and updating any relevant tests or translations if
applicable; keep the onClick handler handleShare and the button markup unchanged
except for replacing the literal strings with the constant.
- Line 175: The toast.success call in DoubtCard.tsx uses a hardcoded string
("Link copied!"); extract that string into a named constant (e.g.,
COPY_SUCCESS_MESSAGE or LINK_COPIED_MESSAGE) and replace the literal in the
toast.success(...) call inside the DoubtCard component (the handler that
triggers the copy toast). Define the constant at the top of the file (or import
from a shared constants module) so it can be reused and follows the project's
constant naming conventions.
🪄 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: a9b11c05-e9f0-476d-a0cf-0a2058079f12
📒 Files selected for processing (18)
.github/workflows/gssoc-auto-labeler.ymlsrc/__tests__/api/doubts-id.test.tssrc/__tests__/api/doubts.test.tssrc/__tests__/api/notifications-test-seed.test.tssrc/__tests__/api/replies-action.test.tssrc/__tests__/api/replies-auto-transition.test.tssrc/__tests__/api/replies-vote.test.tssrc/__tests__/api/rooms-members.test.tssrc/__tests__/components/DoubtCard.test.tsxsrc/__tests__/configs/db.test.tssrc/__tests__/lib/membership-guard.test.tssrc/app/api/doubts/[id]/route.tssrc/app/api/rooms/members/route.tssrc/app/doubts/[id]/DoubtPermalinkClient.tsxsrc/app/doubts/[id]/page.tsxsrc/components/DoubtCard.tsxsrc/components/DoubtRepliesModal.tsxsrc/lib/notifications/service.ts
Comment on lines
+64
to
+84
| // Interaction flags | ||
| const { searchParams } = new URL(req.url); | ||
| const userName = searchParams.get("userName"); | ||
| const userLikesCheck = userName || email; | ||
|
|
||
| let hasLiked = false; | ||
| let hasBookmarked = false; | ||
|
|
||
| if (userLikesCheck) { | ||
| const [likeRecord] = await db | ||
| .select() | ||
| .from(likesTable) | ||
| .where( | ||
| and( | ||
| eq(likesTable.userName, userLikesCheck), | ||
| eq(likesTable.doubtId, doubtId) | ||
| ) | ||
| ) | ||
| .limit(1); | ||
| hasLiked = !!likeRecord; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how the userName param is used in client components and whether there's validation
# Search for API calls to /api/doubts/[id] with userName param
rg -n -C3 -g '*.tsx' -g '*.ts' 'userName.*param|searchParams.*userName|/api/doubts/.*userName'
# Search for anonymous user handling
rg -n -C3 -g '*.tsx' -g '*.ts' 'anonymous.*user|anonymousUser'Repository: knoxiboy/DoubtDesk
Length of output: 9069
Validate/scoping for userName used in hasLiked to prevent identifier probing
In src/app/api/doubts/[id]/route.ts, userName is taken from searchParams.get("userName") and used via userLikesCheck = userName || email to query the like record. The client passes localStorage.getItem("anonymous_user") as the userName query param (DoubtPermalinkClient.tsx), so hasLiked currently depends on a caller-supplied identifier. Unless the server validates that this identifier belongs to the current requester (or is a signed/opaque anonymous token), callers can probe arbitrary userName values.
- Remove trust in caller-supplied
userNamefor like checks: derive the identifier server-side from the authenticated session, or validate a signed/opaque anonymous identity token before using it in the lookup. - Make the API contract match the intended “anonymous_user” behavior (either enforce
userNameis only the validated anonymous id, or introduce a dedicated validated anonymous parameter/token).
🤖 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/`[id]/route.ts around lines 64 - 84, The like-check trusts
a caller-supplied query param (userName) via userLikesCheck and uses it to set
hasLiked; instead derive the identity server-side or validate a signed/opaque
anonymous token before using it in the like lookup. Update the route handler to
ignore unvalidated searchParams.get("userName"), instead obtain the user
identifier from the authenticated session (or verify a signed anonymous token)
and use that validated id in the db query (replace userLikesCheck with the
server-derived id when querying likesTable for doubtId), and adjust the API
contract / DoubtPermalinkClient.tsx to send only the validated token or nothing
if the server will derive identity. Ensure the handler rejects or treats as
unauthenticated any requests with invalid/unsigned anonymous identifiers.
Comment on lines
+23
to
+35
| const fetchDoubt = async () => { | ||
| try { | ||
| const userName = localStorage.getItem("anonymous_user"); | ||
| const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : ""); | ||
| const res = await fetch(url); | ||
| if (res.ok) { | ||
| const data = await res.json(); | ||
| setDoubt(data); | ||
| } | ||
| } catch (error) { | ||
| console.error("Failed to fetch doubt details:", error); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
Add user-facing error feedback when fetch fails.
The fetch error is only logged to the console (line 33). Users receive no visual indication that loading the doubt details failed. As per coding guidelines, missing error states should be addressed in TSX files.
🚨 Proposed fix to show error toast
+import { toast } from "sonner";
+
const fetchDoubt = async () => {
try {
const userName = localStorage.getItem("anonymous_user");
const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : "");
const res = await fetch(url);
if (res.ok) {
const data = await res.json();
setDoubt(data);
+ } else {
+ toast.error("Failed to load doubt details");
}
} catch (error) {
console.error("Failed to fetch doubt details:", error);
+ toast.error("Failed to load doubt details");
}
};📝 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
| const fetchDoubt = async () => { | |
| try { | |
| const userName = localStorage.getItem("anonymous_user"); | |
| const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : ""); | |
| const res = await fetch(url); | |
| if (res.ok) { | |
| const data = await res.json(); | |
| setDoubt(data); | |
| } | |
| } catch (error) { | |
| console.error("Failed to fetch doubt details:", error); | |
| } | |
| }; | |
| import { toast } from "sonner"; | |
| const fetchDoubt = async () => { | |
| try { | |
| const userName = localStorage.getItem("anonymous_user"); | |
| const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : ""); | |
| const res = await fetch(url); | |
| if (res.ok) { | |
| const data = await res.json(); | |
| setDoubt(data); | |
| } else { | |
| toast.error("Failed to load doubt details"); | |
| } | |
| } catch (error) { | |
| console.error("Failed to fetch doubt details:", error); | |
| toast.error("Failed to load doubt details"); | |
| } | |
| }; |
🤖 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/doubts/`[id]/DoubtPermalinkClient.tsx around lines 23 - 35, The
fetchDoubt function currently only logs failures to console; add user-facing
feedback by introducing an error state or calling the app's toast/notification
helper from the catch block so users see a message when fetching initialDoubt.id
fails. Specifically, add a state (e.g., const [fetchError, setFetchError] =
useState<string | null>(null)) and in fetchDoubt's catch call
setFetchError(error.message || "Failed to load doubt") or call the existing
toast/showToast helper with that message, and clear the error when the fetch
succeeds (inside the res.ok branch call setFetchError(null)). Update the
component render to display a brief error UI (toast/banner) when fetchError is
set so users get visible feedback instead of only console.error.
Comment on lines
+37
to
+39
| useEffect(() => { | ||
| fetchDoubt(); | ||
| }, [initialDoubt.id]); |
Contributor
There was a problem hiding this comment.
Add fetchDoubt to the dependency array or memoize it with useCallback.
The useEffect calls fetchDoubt but doesn't include it in the dependency array, violating React's exhaustive-deps rule. This creates a stale closure: if initialDoubt.id changes, the effect will call an outdated version of fetchDoubt that closes over old props/state.
✅ Recommended fix using useCallback
+import { useEffect, useState, useCallback } from "react";
+
export default function DoubtPermalinkClient({ initialDoubt }: DoubtPermalinkClientProps) {
const [doubt, setDoubt] = useState<any>({
...initialDoubt,
tags: [],
replyCount: initialDoubt.replyCount || 0,
hasLiked: false,
hasBookmarked: false,
});
- const fetchDoubt = async () => {
+ const fetchDoubt = useCallback(async () => {
try {
const userName = localStorage.getItem("anonymous_user");
const url = `/api/doubts/${initialDoubt.id}` + (userName ? `?userName=${encodeURIComponent(userName)}` : "");
const res = await fetch(url);
if (res.ok) {
const data = await res.json();
setDoubt(data);
}
} catch (error) {
console.error("Failed to fetch doubt details:", error);
}
- };
+ }, [initialDoubt.id]);
useEffect(() => {
fetchDoubt();
- }, [initialDoubt.id]);
+ }, [fetchDoubt]);🤖 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/doubts/`[id]/DoubtPermalinkClient.tsx around lines 37 - 39, The
useEffect currently calls fetchDoubt but doesn’t include it in the dependency
array, which can close over stale values; fix by either adding fetchDoubt to the
dependency array of the useEffect or memoizing fetchDoubt with useCallback (so
its identity only changes when its true dependencies change), referencing the
existing fetchDoubt function and the effect that depends on initialDoubt.id;
ensure the chosen approach updates the dependency list to include fetchDoubt (or
the stable callback) so the effect always uses the current closure.
Owner
|
Automated Detailed Review
Please address the above items and request a review again. |
Owner
|
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! |
Owner
|
Hi @angelina-2206! 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:
...
File File Details<...
⚙️ CI/CD PipelineYour PR currently has some failing checks:
Note: We have recently fixed database configuration and This is an automated review assessment. Please address the feedback above to move your PR forward. |
Owner
|
knoxiboy
added
merge-conflict
Owner
|
Hi! Thanks for working on this. We recently cleaned up our styling configurations and relocated some layouts, which has introduced merge conflicts in your changed files. Could you rebase this branch against main, resolve the conflicts, and update the PR? Thank you! |
github-actions
Bot
added
the
merge-conflict
knoxiboy
force-pushed
the
feat/doubt-permalink-page
branch
from
July 24, 2026 11:22
7c00cde to
a535cb0
Compare
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
github-actions
Bot
added
size/xs
and removed
gssoc'26
github-actions
Bot
added
gssoc'26
github-actions
Bot
added
gssoc:approved
knoxiboy
added
type:feature
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
Implements dynamic permalink routes, clipboard copy sharing, and notification link refactoring.
Resolves #447.
Changes
/api/doubts/[id]route/doubts/[id]detail pageDoubtCardcomponent with Share button and copy-to-clipboard functionalityCodeAnt-AI Description
Add a shareable doubt permalink page with inline discussion
What Changed
/doubts/[id]page with the doubt and replies shown togetherImpact
✅ Easier doubt sharing✅ Fewer broken notification links✅ Smoother classroom discussions💡 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.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests