Searchbar fix#57
Open
alSN0W wants to merge 46 commits into
Open
Conversation
Review engagement
added vote button
fixed displaying the user votes incorrectly issue
Styled Voting buttons and updated migration files
removed useVote as it's useless for now
added pagination type and utils
This reverts commit f2ac58d.
Revert "added pagination type and utils"
made changes recommended by code-rabbit
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
some code-rabbit changes
documentation changes
removed all the useless routes and functions
Revert "removed all the useless routes and functions"
Revert "Revert "removed all the useless routes and functions""
final checks
updated migration with rls for votes table
fixed searchbar bug and implemented instant search results
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…to searchbar-fix Added some coderabbit fixes to the searchbar , and now merging it into the searchbar-fix branch to test it out. Hopefully this will fix the searchbar issues that we have been having
WalkthroughThis PR introduces a complete review pagination and voting system, replacing legacy ratings endpoints with a reviews-focused architecture. It upgrades React and Next.js dependencies, refactors filter debouncing, enables sign-in magic links, and updates route parameter handling to work with async props in the App Router. ChangesReview Pagination & Voting System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
This was referenced May 25, 2026
|
@Ashish-Kumar-Dash is attempting to deploy a commit to the OpenLake_Website Team on Vercel. A member of the Team first needs to authorize it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/types/reviews.tsx (1)
4-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
Review.idto keep the review/vote contract intact.Line 4 says this type matches the DB schema, but
idis missing. That breaks alignment with vote linkage (review_id) and review list identity handling.Suggested fix
export interface Review { + id: string; courseId: string; courseName: string; courseCode: string;🤖 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/reviews.tsx` around lines 4 - 21, The Review interface is missing the id field which breaks linkage with votes (review_id) and list identity; update the Review type by adding an id: string property to the interface so it matches the DB schema and maintains the review/vote contract—locate the Review interface in src/types/reviews.tsx and add the id field alongside courseId, courseName, etc., ensuring any consumers (e.g., vote handling or list keys) can rely on review.id.src/components/courses/course_page/CoursePageReviews.tsx (1)
52-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard async state updates against stale
idrequests.When
idchanges during an in-flight query, older responses can still update state and show reviews for the wrong course (or afteridbecomes null).Proposed fix
useEffect(() => { + let cancelled = false; + const fetchReviews = async () => { const targetId = id; if (!targetId) { - setReviews([]); - setLoading(false); + if (!cancelled) { + setReviews([]); + setLoading(false); + } return; } setLoading(true); @@ - if (error) { + if (cancelled) return; + + if (error) { console.error("Error fetching reviews:", error.message); + setReviews([]); } else { setReviews(data || []); } setLoading(false); }; fetchReviews(); + return () => { + cancelled = true; + }; }, [id]);🤖 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/courses/course_page/CoursePageReviews.tsx` around lines 52 - 85, The fetchReviews effect must avoid applying results from an out-of-date query when id changes; capture the current id as a local const (already targetId) and before calling setReviews or setLoading verify that the latest id/ref still equals that captured targetId (or that the component is still mounted), so ignore the response if id has changed or become null; update the fetchReviews function to bail out before updating state when the stored targetId differs from the current id (and ensure the initial null-check still prevents updates when id is falsy).
🧹 Nitpick comments (1)
src/components/courses/Filters.tsx (1)
66-68: ⚡ Quick winRemove or wire up unused sticky-scroll state
filterRef,filterTop, andisStickyare maintained, but no JSX element is bound withref={filterRef}, andisStickyis not used in render. This leaves a scroll listener doing no user-visible work.Suggested cleanup
- const [isSticky, setIsSticky] = useState<boolean>(false); - const filterRef = useRef<HTMLDivElement>(null); - const [filterTop, setFilterTop] = useState<number>(0); @@ - // Calculate initial position and handle scroll - useEffect(() => { - const handleScroll = () => { - if (filterRef.current) { - const rect = filterRef.current.getBoundingClientRect(); - const scrollY = window.scrollY; - - if (filterTop === 0 && !isSticky) { - setFilterTop(scrollY + rect.top); - } - - setIsSticky(scrollY > filterTop - 80); - } - }; - - if (filterRef.current && filterTop === 0) { - const rect = filterRef.current.getBoundingClientRect(); - setFilterTop(window.scrollY + rect.top); - } - - window.addEventListener("scroll", handleScroll, { passive: true }); - return () => window.removeEventListener("scroll", handleScroll); - }, [filterTop, isSticky]);Also applies to: 111-133
🤖 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/courses/Filters.tsx` around lines 66 - 68, The component currently creates filterRef, filterTop and isSticky and registers a scroll listener but never binds filterRef to any JSX and never uses isSticky in render, so either remove the unused state/effect or wire them up: either (A) delete filterRef, filterTop, isSticky and the scroll useEffect to remove dead code, or (B) attach ref={filterRef} to the filters container element in the JSX (the element that should become sticky), then use isSticky to toggle a CSS class or inline style for the sticky state (e.g., add className={isSticky ? 'sticky' : ''}) and keep the existing scroll handler that updates setFilterTop/setIsSticky; update any related CSS to implement the visual sticky behavior.
🤖 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 `@package.json`:
- Line 64: The package.json currently upgrades "next" to ^16.1.6 while leaving
"eslint-config-next" and "`@next/swc-wasm-nodejs`" pinned at 13.5.1, causing
tooling/version skew; update the dependency entries for eslint-config-next and
`@next/swc-wasm-nodejs` to versions compatible with Next 16 (or remove the
explicit `@next/swc-wasm-nodejs` entry entirely if not required) so the three
packages align—modify the "eslint-config-next" and "`@next/swc-wasm-nodejs`"
values in package.json to Next 16-compatible releases that match the "next"
^16.1.6 upgrade.
In `@src/components/common/VoteButton.tsx`:
- Around line 123-135: The success branch currently only calls
setCurrentVote(data.vote_type) so voteCount can remain stale; after parsing
response (data) update the local count by reconciling with the server action
(use data.action or data.vote_type) — e.g., compute the delta from the previous
value and call setVoteCount(prev => /* apply delta based on data.action */) — or
alternatively invoke the parent's refresh callback by enhancing onVote(reviewId,
data.vote_type) to accept/return the canonical count and use that to
setVoteCount; update the logic around setCurrentVote, setVoteCount, and the
onVote call to ensure voteCount is rebased from the server result, not only
optimistic state.
In `@src/components/courses/Filters.tsx`:
- Around line 349-354: Replace the clickable icon elements used to remove
filters with accessible buttons: wrap or replace the X icon component (the JSX
element named X) with a <button type="button"> that has an aria-label (e.g.,
"Remove search filter") and the same onClick handler (onFilterChange({
...currentFilters, searchQuery: "" })) to preserve behavior; do the same for the
other filter-remove icons that call onFilterChange (the ones manipulating
currentFilters for other fields) so they are keyboard-focusable and announced by
assistive tech. Ensure styling/utility classes move to the button and that the
button has no default form submit behavior (type="button") and an appropriate
aria-label for each filter type.
- Around line 77-92: The timeout callback currently closes over a stale snapshot
of currentFilters in handleSearchChange causing delayed updates to reintroduce
old values; fix this by using a ref that always holds the latest filters (e.g.,
currentFiltersRef) and update that ref in a useEffect when currentFilters
changes, then inside debouncedSearchRef's setTimeout spread
currentFiltersRef.current instead of the captured currentFilters; alternatively,
make onFilterChange accept an updater function and call onFilterChange(prev =>
({ ...prev, searchQuery: value })) from the timeout — reference
handleSearchChange, debouncedSearchRef, currentFilters, and onFilterChange when
applying the change.
In `@src/hooks/usePaginatedReviews.ts`:
- Around line 40-103: When targetId changes we must reset pagination and prevent
stale in-flight responses from overwriting state: in the effect that watches
targetId, call setCurrentPage(1) and then fetchReviews(1) (instead of using the
previous currentPage), and modify fetchReviews to accept/ use an AbortController
(create one per call, pass its signal to fetch, and abort in the effect cleanup
or before starting a new fetch) and ignore responses if the fetch was aborted;
alternatively tag each request with a local requestId captured in fetchReviews
and only apply state updates when the requestId matches the latest. Ensure
cleanup aborts outstanding requests on unmount/param changes and keep references
to setHasNextPage/setHasPreviousPage etc. as currently used.
In `@src/lib/pagination.ts`:
- Around line 13-15: totalPages can be 0 while currentPage is forced to 1,
violating currentPage <= totalPages for empty results; change the currentPage
calculation in pagination.ts so it respects empty-result semantics by setting
currentPage to 0 when safeTotal (or totalPages) is 0, otherwise clamp page
between 1 and totalPages (i.e., compute currentPage = totalPages === 0 ? 0 :
Math.max(1, Math.min(page, totalPages))), updating any uses of
safeTotal/safeLimit/page/currentPage accordingly.
In `@src/pages/api/ratings/vote/index.ts`:
- Around line 15-24: The request currently only checks presence of review_id but
not shape, causing malformed UUIDs or empty strings (e.g., from
review_ids.split(',')) to reach Supabase and return 500s; add a shared validator
(e.g., isValidReviewId or isValidUUID) used by the POST handler that checks
review_id is a non-empty string and matches UUID v4/standard pattern and return
400 on failure, and in the GET handler filter out blank IDs after splitting
review_ids and reject the whole request if any id is malformed; update usages in
the file (validation around const { review_id, vote_type } = req.body, the GET
review_ids parsing logic, and the other blocks noted) to call the validator
before any .eq() or .in() calls.
- Around line 45-52: The route currently uses votes.auth_id (in the select and
insert) but the votes table only has UNIQUE (review_id, anonymous_id), so remove
all references to auth_id and switch to anonymous_id for reads/writes; replace
the racy read-then-write toggle logic in this handler with a single atomic
server-side operation: create a Postgres function (e.g. toggle_vote(review_id
UUID, anonymous_id TEXT, vote_type TEXT) RETURNS votes or a record) that checks
votes by (review_id, anonymous_id), deletes the row if the existing vote_type
equals the incoming vote_type, otherwise inserts or updates using INSERT ... ON
CONFLICT (review_id, anonymous_id) DO UPDATE SET vote_type = EXCLUDED.vote_type,
then call it from the route via supabase.rpc('toggle_vote', { review_id,
anonymous_id, vote_type }) and use its returned payload; ensure the handler
references the new RPC and the votes table schema names (review_id,
anonymous_id, vote_type) only and remove any .eq('auth_id', ...) and auth_id
fields from .insert().
In `@src/pages/api/reviews/index.ts`:
- Around line 55-66: The current professor lookup in the reviews API uses
target_id as an email and returns it in error messages (see the target_type ===
'professor' branch, .eq('email', target_id.toLowerCase()), lookupError,
professor), which leaks PII and enables enumeration; change the endpoint to
require a non-PII identifier (prefer numeric id or UUID) for professors and
perform a strict format check on target_id before querying (e.g., only accept
/^\d+$/ or a UUID pattern), query .eq('id', parsedId) instead of email, and
replace error returns/logs with a generic message like "Professor not found"
without echoing target_id; ensure lookupError is still surfaced to internal logs
in a safe way (no raw identifier).
In `@src/utils/supabase/server-pages.ts`:
- Around line 18-24: The setAll implementation overwrites existing Set-Cookie
headers; modify setAll (the function that calls res.setHeader('Set-Cookie',
...)) to first read existing cookies via res.getHeader('Set-Cookie') (or
res.getHeaders()) and normalize to an array, then merge that array with the new
cookies produced by cookiesToSet.map(({name,value,options}) =>
serializeCookieHeader(name,value,options)), and finally call
res.setHeader('Set-Cookie', mergedArray) so both existing and Supabase cookies
are preserved.
---
Outside diff comments:
In `@src/components/courses/course_page/CoursePageReviews.tsx`:
- Around line 52-85: The fetchReviews effect must avoid applying results from an
out-of-date query when id changes; capture the current id as a local const
(already targetId) and before calling setReviews or setLoading verify that the
latest id/ref still equals that captured targetId (or that the component is
still mounted), so ignore the response if id has changed or become null; update
the fetchReviews function to bail out before updating state when the stored
targetId differs from the current id (and ensure the initial null-check still
prevents updates when id is falsy).
In `@src/types/reviews.tsx`:
- Around line 4-21: The Review interface is missing the id field which breaks
linkage with votes (review_id) and list identity; update the Review type by
adding an id: string property to the interface so it matches the DB schema and
maintains the review/vote contract—locate the Review interface in
src/types/reviews.tsx and add the id field alongside courseId, courseName, etc.,
ensuring any consumers (e.g., vote handling or list keys) can rely on review.id.
---
Nitpick comments:
In `@src/components/courses/Filters.tsx`:
- Around line 66-68: The component currently creates filterRef, filterTop and
isSticky and registers a scroll listener but never binds filterRef to any JSX
and never uses isSticky in render, so either remove the unused state/effect or
wire them up: either (A) delete filterRef, filterTop, isSticky and the scroll
useEffect to remove dead code, or (B) attach ref={filterRef} to the filters
container element in the JSX (the element that should become sticky), then use
isSticky to toggle a CSS class or inline style for the sticky state (e.g., add
className={isSticky ? 'sticky' : ''}) and keep the existing scroll handler that
updates setFilterTop/setIsSticky; update any related CSS to implement the visual
sticky behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8066329e-cf6c-440a-a1e8-451d891af7e8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
next-env.d.tsnext.config.jspackage.jsonsrc/app/auth/signin/page.tsxsrc/app/courses/[courseId]/page.tsxsrc/components/common/Pagination.tsxsrc/components/common/VoteButton.tsxsrc/components/courses/Filters.tsxsrc/components/courses/course_page/CoursePageReviews.tsxsrc/hooks/usePaginatedReviews.tssrc/lib/pagination.tssrc/lib/withPagination.tssrc/migrations/migration.sqlsrc/pages/api/ratings/route.tssrc/pages/api/ratings/vote/index.tssrc/pages/api/ratings/vote/route.tssrc/pages/api/reviews/index.tssrc/types/pagination.tssrc/types/reviews.tsxsrc/utils/supabase/server-pages.tssrc/utils/supabase/server.tstsconfig.json
💤 Files with no reviewable changes (2)
- src/pages/api/ratings/vote/route.ts
- src/pages/api/ratings/route.ts
| "input-otp": "^1.2.4", | ||
| "lucide-react": "^0.446.0", | ||
| "next": "13.5.1", | ||
| "next": "^16.1.6", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import json, re, urllib.request, urllib.parse
with open("package.json", "r", encoding="utf-8") as f:
deps = json.load(f).get("dependencies", {})
def major(v: str | None):
if not v:
return None
m = re.search(r"\d+", v)
return int(m.group(0)) if m else None
next_v = deps.get("next")
next_major = major(next_v)
print("Local dependency versions:")
for pkg in ["next", "eslint-config-next", "`@next/swc-wasm-nodejs`"]:
v = deps.get(pkg)
print(f" {pkg}: {v}")
print("\nMajor alignment check against `next`:")
for pkg in ["eslint-config-next", "`@next/swc-wasm-nodejs`"]:
v = deps.get(pkg)
if v is None:
print(f" {pkg}: not present (OK if intentionally removed)")
continue
print(f" {pkg}: {'OK' if major(v) == next_major else 'MISMATCH'} (next={next_major}, pkg={major(v)})")
print("\nLatest npm versions:")
for pkg in ["next", "eslint-config-next", "`@next/swc-wasm-nodejs`"]:
enc = urllib.parse.quote(pkg, safe="")
with urllib.request.urlopen(f"https://registry.npmjs.org/{enc}/latest") as r:
latest = json.load(r)["version"]
print(f" {pkg}: {latest}")
print("\nExpected: `eslint-config-next` major should align with `next` major, and stale explicit SWC pins should be removed unless strictly required.")
PYRepository: OpenLake/RateMyCourse
Length of output: 2932
Align eslint-config-next and @next/swc-wasm-nodejs with Next.js 16
package.json uses next: ^16.1.6 but keeps eslint-config-next: 13.5.1 and @next/swc-wasm-nodejs: 13.5.1 pinned to the Next 13 line, creating a dependency/tooling version skew that can break lint/SWC resolution. Update these companion packages to Next 16-compatible versions (or remove the explicit SWC pin if it’s not required).
🤖 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 `@package.json` at line 64, The package.json currently upgrades "next" to
^16.1.6 while leaving "eslint-config-next" and "`@next/swc-wasm-nodejs`" pinned at
13.5.1, causing tooling/version skew; update the dependency entries for
eslint-config-next and `@next/swc-wasm-nodejs` to versions compatible with Next 16
(or remove the explicit `@next/swc-wasm-nodejs` entry entirely if not required) so
the three packages align—modify the "eslint-config-next" and
"`@next/swc-wasm-nodejs`" values in package.json to Next 16-compatible releases
that match the "next" ^16.1.6 upgrade.
Comment on lines
+123
to
+135
| const data = await response.json(); | ||
|
|
||
| if (!data.success) { | ||
| throw new Error(data.error || 'Failed to vote'); | ||
| } | ||
|
|
||
| // Update with server response for vote type | ||
| setCurrentVote(data.vote_type); | ||
|
|
||
| // Callback | ||
| if (onVote) { | ||
| onVote(reviewId, data.vote_type); | ||
| } |
There was a problem hiding this comment.
Reconcile voteCount from the server result, not just currentVote.
On success this only updates currentVote. If the client was stale and the server returns a different outcome than the optimistic path, voteCount stays wrong until a later refresh. Rebase the count from oldCount using data.action here, or trigger a parent refresh after a successful vote.
Suggested fix
const data = await response.json();
if (!data.success) {
throw new Error(data.error || 'Failed to vote');
}
- // Update with server response for vote type
setCurrentVote(data.vote_type);
+ setVoteCount(() => {
+ switch (data.action) {
+ case 'created':
+ return oldCount + (voteType === 'helpful' ? 1 : -1);
+ case 'removed':
+ return oldCount + (voteType === 'helpful' ? -1 : 1);
+ case 'updated':
+ return oldCount + (voteType === 'helpful' ? 2 : -2);
+ default:
+ return oldCount;
+ }
+ });
// Callback
if (onVote) {
onVote(reviewId, data.vote_type);
}🤖 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/common/VoteButton.tsx` around lines 123 - 135, The success
branch currently only calls setCurrentVote(data.vote_type) so voteCount can
remain stale; after parsing response (data) update the local count by
reconciling with the server action (use data.action or data.vote_type) — e.g.,
compute the delta from the previous value and call setVoteCount(prev => /* apply
delta based on data.action */) — or alternatively invoke the parent's refresh
callback by enhancing onVote(reviewId, data.vote_type) to accept/return the
canonical count and use that to setVoteCount; update the logic around
setCurrentVote, setVoteCount, and the onVote call to ensure voteCount is rebased
from the server result, not only optimistic state.
Comment on lines
+77
to
+92
| const handleSearchChange = useCallback((value: string) => { | ||
| setLocalSearchQuery(value); // Update UI immediately | ||
|
|
||
| // Clear previous timeout | ||
| if (debouncedSearchRef.current) { | ||
| clearTimeout(debouncedSearchRef.current); | ||
| } | ||
|
|
||
| // Set new timeout to update parent after 300ms | ||
| debouncedSearchRef.current = setTimeout(() => { | ||
| onFilterChange({ | ||
| ...currentFilters, | ||
| searchQuery: value | ||
| }); | ||
| }, 300); | ||
| }, [currentFilters, onFilterChange]); |
There was a problem hiding this comment.
Debounced search can overwrite newer parent filter state
At Line 87, the timeout callback spreads a captured currentFilters snapshot. If parent filters change during the 300ms window, this delayed update can reintroduce stale values.
Suggested fix
+ const latestFiltersRef = useRef<FiltersState>(currentFilters);
+
+ useEffect(() => {
+ latestFiltersRef.current = currentFilters;
+ }, [currentFilters]);
+
const handleSearchChange = useCallback((value: string) => {
setLocalSearchQuery(value); // Update UI immediately
@@
// Set new timeout to update parent after 300ms
debouncedSearchRef.current = setTimeout(() => {
onFilterChange({
- ...currentFilters,
+ ...latestFiltersRef.current,
searchQuery: value
});
}, 300);
- }, [currentFilters, onFilterChange]);
+ }, [onFilterChange]);📝 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 handleSearchChange = useCallback((value: string) => { | |
| setLocalSearchQuery(value); // Update UI immediately | |
| // Clear previous timeout | |
| if (debouncedSearchRef.current) { | |
| clearTimeout(debouncedSearchRef.current); | |
| } | |
| // Set new timeout to update parent after 300ms | |
| debouncedSearchRef.current = setTimeout(() => { | |
| onFilterChange({ | |
| ...currentFilters, | |
| searchQuery: value | |
| }); | |
| }, 300); | |
| }, [currentFilters, onFilterChange]); | |
| const latestFiltersRef = useRef<FiltersState>(currentFilters); | |
| useEffect(() => { | |
| latestFiltersRef.current = currentFilters; | |
| }, [currentFilters]); | |
| const handleSearchChange = useCallback((value: string) => { | |
| setLocalSearchQuery(value); // Update UI immediately | |
| // Clear previous timeout | |
| if (debouncedSearchRef.current) { | |
| clearTimeout(debouncedSearchRef.current); | |
| } | |
| // Set new timeout to update parent after 300ms | |
| debouncedSearchRef.current = setTimeout(() => { | |
| onFilterChange({ | |
| ...latestFiltersRef.current, | |
| searchQuery: value | |
| }); | |
| }, 300); | |
| }, [onFilterChange]); |
🤖 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/courses/Filters.tsx` around lines 77 - 92, The timeout
callback currently closes over a stale snapshot of currentFilters in
handleSearchChange causing delayed updates to reintroduce old values; fix this
by using a ref that always holds the latest filters (e.g., currentFiltersRef)
and update that ref in a useEffect when currentFilters changes, then inside
debouncedSearchRef's setTimeout spread currentFiltersRef.current instead of the
captured currentFilters; alternatively, make onFilterChange accept an updater
function and call onFilterChange(prev => ({ ...prev, searchQuery: value })) from
the timeout — reference handleSearchChange, debouncedSearchRef, currentFilters,
and onFilterChange when applying the change.
Comment on lines
+349
to
+354
| <X | ||
| className="h-3 w-3 cursor-pointer" | ||
| onClick={() => | ||
| onFilterChange({ ...currentFilters, searchQuery: "" }) | ||
| } | ||
| /> |
There was a problem hiding this comment.
Use real buttons for filter-remove controls
The clickable X icons are mouse-only affordances right now. Use <button type="button"> with an aria-label so keyboard and assistive tech users can remove individual filters.
Suggested pattern
- <X
- className="h-3 w-3 cursor-pointer"
- onClick={() =>
- onFilterChange({ ...currentFilters, searchQuery: "" })
- }
- />
+ <button
+ type="button"
+ aria-label="Remove search filter"
+ onClick={() => onFilterChange({ ...currentFilters, searchQuery: "" })}
+ className="inline-flex items-center"
+ >
+ <X className="h-3 w-3" />
+ </button>Also applies to: 370-373, 385-388, 396-401
🤖 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/courses/Filters.tsx` around lines 349 - 354, Replace the
clickable icon elements used to remove filters with accessible buttons: wrap or
replace the X icon component (the JSX element named X) with a <button
type="button"> that has an aria-label (e.g., "Remove search filter") and the
same onClick handler (onFilterChange({ ...currentFilters, searchQuery: "" })) to
preserve behavior; do the same for the other filter-remove icons that call
onFilterChange (the ones manipulating currentFilters for other fields) so they
are keyboard-focusable and announced by assistive tech. Ensure styling/utility
classes move to the button and that the button has no default form submit
behavior (type="button") and an appropriate aria-label for each filter type.
Comment on lines
+40
to
+103
| const fetchReviews = useCallback(async (page: number) => { | ||
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| try { | ||
| const params = new URLSearchParams({ | ||
| page: page.toString(), | ||
| limit: limit.toString(), | ||
| target_id: targetId, | ||
| target_type: targetType, | ||
| sort_by: sortBy, | ||
| sort_order: sortOrder, | ||
| }); | ||
|
|
||
| const url = `/api/reviews?${params}`; | ||
| const response = await fetch(url); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| console.error('API error response:', errorText); | ||
| throw new Error(`Failed to fetch reviews: ${response.status}`); | ||
| } | ||
|
|
||
| const result: PaginatedResponse<Review> = await response.json(); | ||
|
|
||
| // Check if the response has the expected structure | ||
| if (!result || typeof result !== 'object') { | ||
| throw new Error('Invalid response format'); | ||
| } | ||
|
|
||
| if (!result.success) { | ||
| throw new Error(result.error || 'Failed to fetch reviews'); | ||
| } | ||
|
|
||
| setData(result.data || []); | ||
| setCurrentPage(result.pagination.currentPage); | ||
| setTotalPages(result.pagination.totalPages); | ||
| setTotalItems(result.pagination.totalItems); | ||
| setHasNextPage(result.pagination.hasNextPage); | ||
| setHasPreviousPage(result.pagination.hasPreviousPage); | ||
|
|
||
| } catch (err) { | ||
| console.error('Error fetching reviews:', err); | ||
| setError(err instanceof Error ? err.message : 'An error occurred'); | ||
| // Set empty state on error | ||
| setData([]); | ||
| setTotalPages(0); | ||
| setTotalItems(0); | ||
| setHasNextPage(false); | ||
| setHasPreviousPage(false); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }, [targetId, targetType, limit, sortBy, sortOrder]); | ||
|
|
||
| // Initial fetch - only run when we have a valid targetId | ||
| useEffect(() => { | ||
| if (!targetId) { | ||
| console.warn('usePaginatedReviews: targetId is missing'); | ||
| return; | ||
| } | ||
| fetchReviews(currentPage); | ||
| }, [fetchReviews, currentPage, targetId]); | ||
|
|
There was a problem hiding this comment.
Reset pagination on target change and prevent stale response overwrite.
Line 101 fetches with the previous currentPage after targetId changes, and in-flight requests are not cancelled, so older responses can overwrite newer state.
Proposed fix
- const fetchReviews = useCallback(async (page: number) => {
+ const fetchReviews = useCallback(async (page: number, signal?: AbortSignal) => {
setIsLoading(true);
setError(null);
try {
@@
- const response = await fetch(url);
+ const response = await fetch(url, { signal });
@@
- } catch (err) {
+ } catch (err) {
+ if (err instanceof DOMException && err.name === 'AbortError') {
+ return;
+ }
console.error('Error fetching reviews:', err);
setError(err instanceof Error ? err.message : 'An error occurred');
@@
}, [targetId, targetType, limit, sortBy, sortOrder]);
- // Initial fetch - only run when we have a valid targetId
+ // Reset page when target changes
useEffect(() => {
+ if (!targetId) return;
+ setCurrentPage(1);
+ }, [targetId]);
+
+ // Fetch whenever page/target changes
+ useEffect(() => {
if (!targetId) {
console.warn('usePaginatedReviews: targetId is missing');
return;
}
- fetchReviews(currentPage);
+ const controller = new AbortController();
+ fetchReviews(currentPage, controller.signal);
+ return () => controller.abort();
}, [fetchReviews, currentPage, targetId]);🤖 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/hooks/usePaginatedReviews.ts` around lines 40 - 103, When targetId
changes we must reset pagination and prevent stale in-flight responses from
overwriting state: in the effect that watches targetId, call setCurrentPage(1)
and then fetchReviews(1) (instead of using the previous currentPage), and modify
fetchReviews to accept/ use an AbortController (create one per call, pass its
signal to fetch, and abort in the effect cleanup or before starting a new fetch)
and ignore responses if the fetch was aborted; alternatively tag each request
with a local requestId captured in fetchReviews and only apply state updates
when the requestId matches the latest. Ensure cleanup aborts outstanding
requests on unmount/param changes and keep references to
setHasNextPage/setHasPreviousPage etc. as currently used.
Comment on lines
+13
to
+15
| const totalPages = Math.ceil(safeTotal / safeLimit); | ||
| const currentPage = Math.max(1, Math.min(page, totalPages)); | ||
|
|
There was a problem hiding this comment.
Keep pagination metadata internally consistent for empty results.
When totalItems is 0, totalPages becomes 0 but currentPage becomes 1. This violates currentPage <= totalPages and can create client-side edge-case bugs.
Suggested fix (one consistent convention)
- const totalPages = Math.ceil(safeTotal / safeLimit);
+ const totalPages = Math.max(1, Math.ceil(safeTotal / safeLimit));
const currentPage = Math.max(1, Math.min(page, totalPages));📝 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 totalPages = Math.ceil(safeTotal / safeLimit); | |
| const currentPage = Math.max(1, Math.min(page, totalPages)); | |
| const totalPages = Math.max(1, Math.ceil(safeTotal / safeLimit)); | |
| const currentPage = Math.max(1, Math.min(page, totalPages)); | |
🤖 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/pagination.ts` around lines 13 - 15, totalPages can be 0 while
currentPage is forced to 1, violating currentPage <= totalPages for empty
results; change the currentPage calculation in pagination.ts so it respects
empty-result semantics by setting currentPage to 0 when safeTotal (or
totalPages) is 0, otherwise clamp page between 1 and totalPages (i.e., compute
currentPage = totalPages === 0 ? 0 : Math.max(1, Math.min(page, totalPages))),
updating any uses of safeTotal/safeLimit/page/currentPage accordingly.
Comment on lines
+15
to
+24
| const { review_id, vote_type } = req.body; | ||
|
|
||
| // Validate input | ||
| if (!review_id || !vote_type) { | ||
| return res.status(400).json({ error: 'review_id and vote_type are required' }); | ||
| } | ||
|
|
||
| if (!['helpful', 'unhelpful'].includes(vote_type)) { | ||
| return res.status(400).json({ error: 'vote_type must be "helpful" or "unhelpful"' }); | ||
| } |
There was a problem hiding this comment.
Reject malformed review_id values before querying Supabase.
These branches only check presence, not shape. A bad UUID, or an empty token from review_ids.split(','), will fall through to .eq() / .in() and come back as a 500 instead of a 400. Please add a shared validator for review_id and filter out blank IDs in the GET path.
Also applies to: 132-155, 184-202
🤖 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/pages/api/ratings/vote/index.ts` around lines 15 - 24, The request
currently only checks presence of review_id but not shape, causing malformed
UUIDs or empty strings (e.g., from review_ids.split(',')) to reach Supabase and
return 500s; add a shared validator (e.g., isValidReviewId or isValidUUID) used
by the POST handler that checks review_id is a non-empty string and matches UUID
v4/standard pattern and return 400 on failure, and in the GET handler filter out
blank IDs after splitting review_ids and reject the whole request if any id is
malformed; update usages in the file (validation around const { review_id,
vote_type } = req.body, the GET review_ids parsing logic, and the other blocks
noted) to call the validator before any .eq() or .in() calls.
Comment on lines
+45
to
+52
| // Check if user already voted (by auth_id to prevent duplicate anonymous_ids) | ||
| const { data: existingVote, error: checkError } = await supabase | ||
| .from('votes') | ||
| .select('id, vote_type') | ||
| .eq('review_id', review_id) | ||
| .eq('auth_id', auth_id) // ← Check by real user, not anonymous_id | ||
| .single(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect SQL definitions touching the votes table / relevant keys.
fd -i '\.sql$' src | xargs -r rg -n -C3 'votes|review_id|auth_id|unique|constraint'
# Inspect this route's current read-then-write flow.
rg -n -C3 "single\\(|from\\('votes'\\)|insert\\(|update\\(|delete\\(" src/pages/api/ratings/vote/index.tsRepository: OpenLake/RateMyCourse
Length of output: 7701
Fix vote write path to match the votes schema and be conflict-safe.
- The
votestable definesUNIQUE (review_id, anonymous_id)(noauth_idcolumn), but this route queries/inserts usingvotes.auth_id(.eq('auth_id', auth_id)and.insert({ ..., auth_id, ... })). This will break the endpoint and also makes the.single()“existing vote” check unreliable. - The current read-then-write toggle (select → delete/update/insert) is still racy under concurrent
POSTs; it can hit unique-violation errors for the existing(review_id, anonymous_id)key. Replace the flow with a single atomic SQL/RPC that performs the toggle based on(review_id, anonymous_id)(delete if samevote_type, otherwise insert/update viaON CONFLICT).
🤖 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/pages/api/ratings/vote/index.ts` around lines 45 - 52, The route
currently uses votes.auth_id (in the select and insert) but the votes table only
has UNIQUE (review_id, anonymous_id), so remove all references to auth_id and
switch to anonymous_id for reads/writes; replace the racy read-then-write toggle
logic in this handler with a single atomic server-side operation: create a
Postgres function (e.g. toggle_vote(review_id UUID, anonymous_id TEXT, vote_type
TEXT) RETURNS votes or a record) that checks votes by (review_id, anonymous_id),
deletes the row if the existing vote_type equals the incoming vote_type,
otherwise inserts or updates using INSERT ... ON CONFLICT (review_id,
anonymous_id) DO UPDATE SET vote_type = EXCLUDED.vote_type, then call it from
the route via supabase.rpc('toggle_vote', { review_id, anonymous_id, vote_type
}) and use its returned payload; ensure the handler references the new RPC and
the votes table schema names (review_id, anonymous_id, vote_type) only and
remove any .eq('auth_id', ...) and auth_id fields from .insert().
Comment on lines
+55
to
+66
| } else if (target_type === 'professor') { | ||
| // For professors, you might use email or name as the code | ||
| // Adjust this based on how you identify professors | ||
| const { data: professor, error: lookupError } = await supabase | ||
| .from('professors') | ||
| .select('id') | ||
| .eq('email', target_id.toLowerCase()) | ||
| .single(); | ||
|
|
||
| if (lookupError || !professor) { | ||
| return { query: null, error: `Professor with identifier "${target_id}" not found` }; | ||
| } |
There was a problem hiding this comment.
Avoid email-based professor lookup via target_id query param.
Using email as a URL query identifier leaks PII into logs and enables enumeration. Also avoid echoing raw identifiers in error messages.
Suggested safer fallback
} else if (target_type === 'professor') {
- // For professors, you might use email or name as the code
- // Adjust this based on how you identify professors
- const { data: professor, error: lookupError } = await supabase
- .from('professors')
- .select('id')
- .eq('email', target_id.toLowerCase())
- .single();
-
- if (lookupError || !professor) {
- return { query: null, error: `Professor with identifier "${target_id}" not found` };
- }
-
- actualTargetId = professor.id;
+ return { query: null, error: 'Professor target_id must be a UUID' };
}🤖 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/pages/api/reviews/index.ts` around lines 55 - 66, The current professor
lookup in the reviews API uses target_id as an email and returns it in error
messages (see the target_type === 'professor' branch, .eq('email',
target_id.toLowerCase()), lookupError, professor), which leaks PII and enables
enumeration; change the endpoint to require a non-PII identifier (prefer numeric
id or UUID) for professors and perform a strict format check on target_id before
querying (e.g., only accept /^\d+$/ or a UUID pattern), query .eq('id',
parsedId) instead of email, and replace error returns/logs with a generic
message like "Professor not found" without echoing target_id; ensure lookupError
is still surfaced to internal logs in a safe way (no raw identifier).
Comment on lines
+18
to
+24
| setAll(cookiesToSet) { | ||
| res.setHeader( | ||
| 'Set-Cookie', | ||
| cookiesToSet.map(({ name, value, options }) => | ||
| serializeCookieHeader(name, value, options) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant section
sed -n '1,120p' src/utils/supabase/server-pages.ts | nl -ba
# Search for other Set-Cookie header usage across the repo
rg -n "setHeader\\(['\"]Set-Cookie['\"]|getHeader\\(['\"]Set-Cookie['\"]|Set-Cookie" -S .Repository: OpenLake/RateMyCourse
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== server-pages.ts (lines 1-140 with numbers) ==="
sed -n '1,140p' src/utils/supabase/server-pages.ts | cat -n
echo "=== Search for Set-Cookie usage in the repo ==="
rg -n --hidden --no-ignore-vcs "Set-Cookie" .Repository: OpenLake/RateMyCourse
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== server-pages.ts line count ==="
wc -l src/utils/supabase/server-pages.ts
echo "=== server-pages.ts full content (numbered) ==="
cat -n src/utils/supabase/server-pages.ts
echo "=== Set-Cookie usage in project source (excluding node_modules) ==="
rg -n --hidden --no-ignore-vcs "Set-Cookie" --glob '!node_modules/**' .Repository: OpenLake/RateMyCourse
Length of output: 1382
Preserve existing Set-Cookie headers when Supabase sets cookies
setAll() currently uses res.setHeader('Set-Cookie', ...), which replaces any existing Set-Cookie values already present on the response. Merge the new Supabase cookies with the existing header values instead.
Suggested fix
setAll(cookiesToSet) {
- res.setHeader(
- 'Set-Cookie',
- cookiesToSet.map(({ name, value, options }) =>
- serializeCookieHeader(name, value, options)
- )
- )
+ const existing = res.getHeader('Set-Cookie')
+ const existingCookies = Array.isArray(existing)
+ ? existing
+ : existing
+ ? [String(existing)]
+ : []
+
+ res.setHeader('Set-Cookie', [
+ ...existingCookies,
+ ...cookiesToSet.map(({ name, value, options }) =>
+ serializeCookieHeader(name, value, options)
+ ),
+ ])
}📝 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
| setAll(cookiesToSet) { | |
| res.setHeader( | |
| 'Set-Cookie', | |
| cookiesToSet.map(({ name, value, options }) => | |
| serializeCookieHeader(name, value, options) | |
| ) | |
| ) | |
| setAll(cookiesToSet) { | |
| const existing = res.getHeader('Set-Cookie') | |
| const existingCookies = Array.isArray(existing) | |
| ? existing | |
| : existing | |
| ? [String(existing)] | |
| : [] | |
| res.setHeader('Set-Cookie', [ | |
| ...existingCookies, | |
| ...cookiesToSet.map(({ name, value, options }) => | |
| serializeCookieHeader(name, value, options) | |
| ), | |
| ]) | |
| } |
🤖 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/utils/supabase/server-pages.ts` around lines 18 - 24, The setAll
implementation overwrites existing Set-Cookie headers; modify setAll (the
function that calls res.setHeader('Set-Cookie', ...)) to first read existing
cookies via res.getHeader('Set-Cookie') (or res.getHeaders()) and normalize to
an array, then merge that array with the new cookies produced by
cookiesToSet.map(({name,value,options}) =>
serializeCookieHeader(name,value,options)), and finally call
res.setHeader('Set-Cookie', mergedArray) so both existing and Supabase cookies
are preserved.
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.
Summary by CodeRabbit
New Features
Chores
Bug Fixes