-
Notifications
You must be signed in to change notification settings - Fork 20
fix: professor filter by name and department #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis PR enhances the ItemList component with comprehensive sorting and filtering capabilities, adding asynchronous professor rating data aggregation from a backend, improved difficulty mapping logic, multi-criterion filtering (search, department, difficulty, rating threshold), department-based grouping, and supporting UI updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as ItemList Component
participant Backend as Backend (Supabase)
participant Cache as Data Cache
participant Filter as Filter Engine
participant Renderer as View Renderer
Component->>Backend: Fetch professor ratings data
Backend-->>Component: Return ratings with metadata
Component->>Component: Aggregate ratings by professor<br/>(compute avg: overall, workload, difficulty)
Component->>Cache: Store aggregated professor data
Component->>Filter: Apply filters (search, department,<br/>difficulty, rating threshold)
Filter->>Filter: Process query checks across<br/>courses/professors
Filter->>Filter: Department lookup by ID
Filter->>Filter: Numeric difficulty range matching
Filter-->>Component: Return filtered items
Component->>Component: Sort by selected criterion<br/>& department order (if multi-dept)
Component->>Component: Group courses by department<br/>(preserve master order)
Component->>Renderer: Render grouped/flat view
Renderer-->>Component: Display organized results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/components/courses/ItemList.tsx`:
- Around line 157-159: The minimum-rating check in ItemList.tsx incorrectly
allows unrated items through because it only applies when item.overall_rating >
0; change the logic in the component that filters items (look for the block
using item.overall_rating and filters.rating) to treat missing/unrated ratings
as 0 and perform the comparison whenever a rating threshold is set (e.g., if
filters.rating > 0, compare (normalized item rating || 0) < filters.rating and
return false when below threshold). Ensure you update the condition so unrated
(null/undefined/0) items are excluded when filters.rating > 0.
- Around line 65-115: The effect in useEffect (fetchProfessorAverages) can leave
stale itemsWithAvg when professors is empty and may set state after
type/professors change or unmount; update the effect to immediately clear
itemsWithAvg (call setItemsWithAvg([])) and setIsAggregating(false) when
professors.length === 0, and inside fetchProfessorAverages use a cancellation
flag (e.g., let cancelled = false; check !cancelled before any setItemsWithAvg
or setIsAggregating) and return a cleanup function that flips cancelled to true
to prevent late async updates when type/professors change or component unmounts.
- Around line 165-181: The sort dropdown isn't applied because sorting is not
performed inside the memo that produces filteredItems and sortBy is not included
in that memo's dependency list; update the useMemo that returns filteredItems to
apply sorting based on the sortBy value (handle cases like review_count → your
actual review-count field) before returning result, include sortBy in the
dependency array, and ensure the special-case department ordering logic
(currently using departmentProperties and Course casting) remains part of that
post-filter sort pipeline so filteredItems reflects the selected sort option.
| useEffect(() => { | ||
| if (type === 'professor' && professors.length > 0) { | ||
| if (type !== "professor" || professors.length === 0) return; | ||
|
|
||
| const fetchProfessorAverages = async () => { | ||
| setIsAggregating(true); | ||
|
|
||
| const fetchAverages = async () => { | ||
| const ids = professors.map((i) => i.id); | ||
| if (ids.length === 0) { | ||
| setItemsWithAvg(professors); | ||
| setIsAggregating(false); | ||
| return; | ||
| } | ||
| const ids = professors.map((p) => p.id); | ||
|
|
||
| const { data: ratings, error } = await supabase | ||
| .from("ratings") | ||
| .select("target_id, overall_rating, workload_rating, difficulty_rating") | ||
| .eq("target_type", "professor") // Fetch ratings for professors | ||
| .in("target_id", ids); | ||
|
|
||
| if (error) { | ||
| console.error("Error fetching professor averages:", error.message); | ||
| setItemsWithAvg(professors); // fallback to static data | ||
| setIsAggregating(false); | ||
| return; | ||
| } | ||
| const { data: ratings, error } = await supabase | ||
| .from("ratings") | ||
| .select("target_id, overall_rating, workload_rating, difficulty_rating") | ||
| .eq("target_type", "professor") | ||
| .in("target_id", ids); | ||
|
|
||
| if (error) { | ||
| console.error("Failed to load professor ratings:", error); | ||
| setItemsWithAvg(professors); // fallback | ||
| setIsAggregating(false); | ||
| return; | ||
| } | ||
|
|
||
| // Aggregate averages | ||
| const averages = (ratings || []).reduce((acc, r) => { | ||
| const averages = (ratings || []).reduce( | ||
| (acc, r) => { | ||
| if (!acc[r.target_id]) { | ||
| acc[r.target_id] = { overall: [], workload: [], difficulty: [] }; | ||
| } | ||
| if (r.overall_rating !== null) acc[r.target_id].overall.push(r.overall_rating); | ||
| if (r.workload_rating !== null) acc[r.target_id].workload.push(r.workload_rating); | ||
| if (r.difficulty_rating !== null) acc[r.target_id].difficulty.push(r.difficulty_rating); | ||
| return acc; | ||
| }, {} as Record<string, { overall: number[]; workload: number[]; difficulty: number[] }>); | ||
|
|
||
| // Merge averages with professors | ||
| const calculateAverage = (arr: number[]) => arr.length > 0 ? arr.reduce((a, b) => a + b, 0) / arr.length : 0; | ||
|
|
||
| const merged = professors.map((prof) => { | ||
| const avgData = averages[prof.id]; | ||
| return { | ||
| ...prof, | ||
| overall_rating: calculateAverage(avgData?.overall || []), | ||
| workload_rating: calculateAverage(avgData?.workload || []), | ||
| difficulty_rating: calculateAverage(avgData?.difficulty || []), | ||
| }; | ||
| }); | ||
|
|
||
| setItemsWithAvg(merged); | ||
| setIsAggregating(false); | ||
| }; | ||
| }, | ||
| {} as Record<string, { overall: number[]; workload: number[]; difficulty: number[] }>, | ||
| ); | ||
|
|
||
| const calcAvg = (arr: number[]) => (arr.length > 0 ? arr.reduce((a, b) => a + b, 0) / arr.length : 0); | ||
|
|
||
| const merged = professors.map((prof) => { | ||
| const avg = averages[prof.id]; | ||
| return { | ||
| ...prof, | ||
| overall_rating: calcAvg(avg?.overall || []), | ||
| workload_rating: calcAvg(avg?.workload || []), | ||
| difficulty_rating: calcAvg(avg?.difficulty || []), | ||
| }; | ||
| }); | ||
|
|
||
| fetchAverages(); | ||
| } else if (type === 'professor') { | ||
| // Handle case where professors array is empty | ||
| setItemsWithAvg(professors); | ||
| } | ||
| }, [type, professors]); // Run when professors array changes | ||
| setItemsWithAvg(merged); | ||
| setIsAggregating(false); | ||
| }; | ||
|
|
||
| fetchProfessorAverages(); | ||
| }, [type, professors]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent stale professor lists during async aggregation.
Line 66 returns early when professors is empty, leaving stale itemsWithAvg from a prior type. Also, the async fetch can still set state after type changes or unmount, which can overwrite course results. Clear the list on empty and guard state updates with a cancellation flag.
🐛 Suggested fix
useEffect(() => {
- if (type !== "professor" || professors.length === 0) return;
+ if (type !== "professor") return;
+ if (professors.length === 0) {
+ setItemsWithAvg([]);
+ return;
+ }
+
+ let cancelled = false;
const fetchProfessorAverages = async () => {
setIsAggregating(true);
@@
if (error) {
console.error("Failed to load professor ratings:", error);
- setItemsWithAvg(professors); // fallback
- setIsAggregating(false);
+ if (!cancelled) {
+ setItemsWithAvg(professors); // fallback
+ setIsAggregating(false);
+ }
return;
}
@@
- setItemsWithAvg(merged);
- setIsAggregating(false);
+ if (!cancelled) {
+ setItemsWithAvg(merged);
+ setIsAggregating(false);
+ }
};
fetchProfessorAverages();
+ return () => {
+ cancelled = true;
+ };
}, [type, professors]);📝 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.
| useEffect(() => { | |
| if (type === 'professor' && professors.length > 0) { | |
| if (type !== "professor" || professors.length === 0) return; | |
| const fetchProfessorAverages = async () => { | |
| setIsAggregating(true); | |
| const fetchAverages = async () => { | |
| const ids = professors.map((i) => i.id); | |
| if (ids.length === 0) { | |
| setItemsWithAvg(professors); | |
| setIsAggregating(false); | |
| return; | |
| } | |
| const ids = professors.map((p) => p.id); | |
| const { data: ratings, error } = await supabase | |
| .from("ratings") | |
| .select("target_id, overall_rating, workload_rating, difficulty_rating") | |
| .eq("target_type", "professor") // Fetch ratings for professors | |
| .in("target_id", ids); | |
| if (error) { | |
| console.error("Error fetching professor averages:", error.message); | |
| setItemsWithAvg(professors); // fallback to static data | |
| setIsAggregating(false); | |
| return; | |
| } | |
| const { data: ratings, error } = await supabase | |
| .from("ratings") | |
| .select("target_id, overall_rating, workload_rating, difficulty_rating") | |
| .eq("target_type", "professor") | |
| .in("target_id", ids); | |
| if (error) { | |
| console.error("Failed to load professor ratings:", error); | |
| setItemsWithAvg(professors); // fallback | |
| setIsAggregating(false); | |
| return; | |
| } | |
| // Aggregate averages | |
| const averages = (ratings || []).reduce((acc, r) => { | |
| const averages = (ratings || []).reduce( | |
| (acc, r) => { | |
| if (!acc[r.target_id]) { | |
| acc[r.target_id] = { overall: [], workload: [], difficulty: [] }; | |
| } | |
| if (r.overall_rating !== null) acc[r.target_id].overall.push(r.overall_rating); | |
| if (r.workload_rating !== null) acc[r.target_id].workload.push(r.workload_rating); | |
| if (r.difficulty_rating !== null) acc[r.target_id].difficulty.push(r.difficulty_rating); | |
| return acc; | |
| }, {} as Record<string, { overall: number[]; workload: number[]; difficulty: number[] }>); | |
| // Merge averages with professors | |
| const calculateAverage = (arr: number[]) => arr.length > 0 ? arr.reduce((a, b) => a + b, 0) / arr.length : 0; | |
| const merged = professors.map((prof) => { | |
| const avgData = averages[prof.id]; | |
| return { | |
| ...prof, | |
| overall_rating: calculateAverage(avgData?.overall || []), | |
| workload_rating: calculateAverage(avgData?.workload || []), | |
| difficulty_rating: calculateAverage(avgData?.difficulty || []), | |
| }; | |
| }); | |
| setItemsWithAvg(merged); | |
| setIsAggregating(false); | |
| }; | |
| }, | |
| {} as Record<string, { overall: number[]; workload: number[]; difficulty: number[] }>, | |
| ); | |
| const calcAvg = (arr: number[]) => (arr.length > 0 ? arr.reduce((a, b) => a + b, 0) / arr.length : 0); | |
| const merged = professors.map((prof) => { | |
| const avg = averages[prof.id]; | |
| return { | |
| ...prof, | |
| overall_rating: calcAvg(avg?.overall || []), | |
| workload_rating: calcAvg(avg?.workload || []), | |
| difficulty_rating: calcAvg(avg?.difficulty || []), | |
| }; | |
| }); | |
| fetchAverages(); | |
| } else if (type === 'professor') { | |
| // Handle case where professors array is empty | |
| setItemsWithAvg(professors); | |
| } | |
| }, [type, professors]); // Run when professors array changes | |
| setItemsWithAvg(merged); | |
| setIsAggregating(false); | |
| }; | |
| fetchProfessorAverages(); | |
| }, [type, professors]); | |
| useEffect(() => { | |
| if (type !== "professor") return; | |
| if (professors.length === 0) { | |
| setItemsWithAvg([]); | |
| return; | |
| } | |
| let cancelled = false; | |
| const fetchProfessorAverages = async () => { | |
| setIsAggregating(true); | |
| const ids = professors.map((p) => p.id); | |
| const { data: ratings, error } = await supabase | |
| .from("ratings") | |
| .select("target_id, overall_rating, workload_rating, difficulty_rating") | |
| .eq("target_type", "professor") | |
| .in("target_id", ids); | |
| if (error) { | |
| console.error("Failed to load professor ratings:", error); | |
| if (!cancelled) { | |
| setItemsWithAvg(professors); // fallback | |
| setIsAggregating(false); | |
| } | |
| return; | |
| } | |
| const averages = (ratings || []).reduce( | |
| (acc, r) => { | |
| if (!acc[r.target_id]) { | |
| acc[r.target_id] = { overall: [], workload: [], difficulty: [] }; | |
| } | |
| if (r.overall_rating !== null) acc[r.target_id].overall.push(r.overall_rating); | |
| if (r.workload_rating !== null) acc[r.target_id].workload.push(r.workload_rating); | |
| if (r.difficulty_rating !== null) acc[r.target_id].difficulty.push(r.difficulty_rating); | |
| return acc; | |
| }, | |
| {} as Record<string, { overall: number[]; workload: number[]; difficulty: number[] }>, | |
| ); | |
| const calcAvg = (arr: number[]) => (arr.length > 0 ? arr.reduce((a, b) => a + b, 0) / arr.length : 0); | |
| const merged = professors.map((prof) => { | |
| const avg = averages[prof.id]; | |
| return { | |
| ...prof, | |
| overall_rating: calcAvg(avg?.overall || []), | |
| workload_rating: calcAvg(avg?.workload || []), | |
| difficulty_rating: calcAvg(avg?.difficulty || []), | |
| }; | |
| }); | |
| if (!cancelled) { | |
| setItemsWithAvg(merged); | |
| setIsAggregating(false); | |
| } | |
| }; | |
| fetchProfessorAverages(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [type, professors]); |
🤖 Prompt for AI Agents
In `@src/components/courses/ItemList.tsx` around lines 65 - 115, The effect in
useEffect (fetchProfessorAverages) can leave stale itemsWithAvg when professors
is empty and may set state after type/professors change or unmount; update the
effect to immediately clear itemsWithAvg (call setItemsWithAvg([])) and
setIsAggregating(false) when professors.length === 0, and inside
fetchProfessorAverages use a cancellation flag (e.g., let cancelled = false;
check !cancelled before any setItemsWithAvg or setIsAggregating) and return a
cleanup function that flips cancelled to true to prevent late async updates when
type/professors change or component unmounts.
| // Minimum overall rating | ||
| if (item.overall_rating > 0 && item.overall_rating < filters.rating) { | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minimum rating filter lets unrated items through.
The check only filters when overall_rating > 0, so unrated items always pass even when filters.rating > 0. Normalize to 0 and compare whenever the threshold is set.
🐛 Suggested fix
- if (item.overall_rating > 0 && item.overall_rating < filters.rating) {
+ const overall = item.overall_rating ?? 0;
+ if (filters.rating > 0 && overall < filters.rating) {
return false;
}📝 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.
| // Minimum overall rating | |
| if (item.overall_rating > 0 && item.overall_rating < filters.rating) { | |
| return false; | |
| // Minimum overall rating | |
| const overall = item.overall_rating ?? 0; | |
| if (filters.rating > 0 && overall < filters.rating) { | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In `@src/components/courses/ItemList.tsx` around lines 157 - 159, The
minimum-rating check in ItemList.tsx incorrectly allows unrated items through
because it only applies when item.overall_rating > 0; change the logic in the
component that filters items (look for the block using item.overall_rating and
filters.rating) to treat missing/unrated ratings as 0 and perform the comparison
whenever a rating threshold is set (e.g., if filters.rating > 0, compare
(normalized item rating || 0) < filters.rating and return false when below
threshold). Ensure you update the condition so unrated (null/undefined/0) items
are excluded when filters.rating > 0.
| // Special case: when multiple departments are selected → sort by click order | ||
| if (type === "course" && filters.departments.length > 1) { | ||
| result = result.toSorted((a, b) => { | ||
| const idA = departmentProperties.find((d) => d.name === (a as Course).department)?.id; | ||
| const idB = departmentProperties.find((d) => d.name === (b as Course).department)?.id; | ||
|
|
||
| // Get the index from the filters.departments array (e.g., ['MA', 'EE']) | ||
| const indexA = filters.departments.indexOf(deptIdA); | ||
| const indexB = filters.departments.indexOf(deptIdB); | ||
| const idxA = idA ? filters.departments.indexOf(idA) : -1; | ||
| const idxB = idB ? filters.departments.indexOf(idB) : -1; | ||
|
|
||
| // Handle cases where department might not be found (shouldn't happen) | ||
| if (indexA === -1 && indexB === -1) return 0; | ||
| if (indexA === -1) return 1; | ||
| if (indexB === -1) return -1; | ||
|
|
||
| // This sorts by the click-order index (e.g., 0 for 'MA', 1 for 'EE') | ||
| return indexA - indexB; | ||
| if (idxA === -1 && idxB === -1) return 0; | ||
| if (idxA === -1) return 1; | ||
| if (idxB === -1) return -1; | ||
| return idxA - idxB; | ||
| }); | ||
| return sorted; | ||
| } | ||
| // *** END OF FIX *** | ||
|
|
||
| return filtered; // Return the unsorted (but filtered) list | ||
|
|
||
| }, [itemsWithAvg, filters, type]); // Depend on filters and the processed items | ||
| return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sort dropdown doesn’t affect list order.
sortBy updates the label but never changes filteredItems, so the dropdown appears broken. Apply sorting in this useMemo and add sortBy to its dependency list. (Swap review_count for the actual review-count field in your types.)
🧭 Suggested wiring
- // Special case: when multiple departments are selected → sort by click order
+ // Apply selected sort
+ const byRating = (x: Course | Professor) => x.overall_rating ?? 0;
+ const byDifficulty = (x: Course | Professor) => x.difficulty_rating ?? 0;
+ const byReviews = (x: Course | Professor) =>
+ (x as Course & { review_count?: number }).review_count ?? 0;
+
+ if (sortBy) {
+ result = [...result].sort((a, b) => {
+ switch (sortBy) {
+ case "best-rated":
+ return byRating(b) - byRating(a);
+ case "most-reviewed":
+ return byReviews(b) - byReviews(a);
+ case "easiest":
+ return byDifficulty(a) - byDifficulty(b);
+ case "hardest":
+ return byDifficulty(b) - byDifficulty(a);
+ default:
+ return 0;
+ }
+ });
+ }
+
+ // Special case: when multiple departments are selected → sort by click order
if (type === "course" && filters.departments.length > 1) {
result = result.toSorted((a, b) => {
@@
}
return result;
- }, [itemsWithAvg, filters, type]);
+ }, [itemsWithAvg, filters, type, sortBy]);📝 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.
| // Special case: when multiple departments are selected → sort by click order | |
| if (type === "course" && filters.departments.length > 1) { | |
| result = result.toSorted((a, b) => { | |
| const idA = departmentProperties.find((d) => d.name === (a as Course).department)?.id; | |
| const idB = departmentProperties.find((d) => d.name === (b as Course).department)?.id; | |
| // Get the index from the filters.departments array (e.g., ['MA', 'EE']) | |
| const indexA = filters.departments.indexOf(deptIdA); | |
| const indexB = filters.departments.indexOf(deptIdB); | |
| const idxA = idA ? filters.departments.indexOf(idA) : -1; | |
| const idxB = idB ? filters.departments.indexOf(idB) : -1; | |
| // Handle cases where department might not be found (shouldn't happen) | |
| if (indexA === -1 && indexB === -1) return 0; | |
| if (indexA === -1) return 1; | |
| if (indexB === -1) return -1; | |
| // This sorts by the click-order index (e.g., 0 for 'MA', 1 for 'EE') | |
| return indexA - indexB; | |
| if (idxA === -1 && idxB === -1) return 0; | |
| if (idxA === -1) return 1; | |
| if (idxB === -1) return -1; | |
| return idxA - idxB; | |
| }); | |
| return sorted; | |
| } | |
| // *** END OF FIX *** | |
| return filtered; // Return the unsorted (but filtered) list | |
| }, [itemsWithAvg, filters, type]); // Depend on filters and the processed items | |
| return result; | |
| // Apply selected sort | |
| const byRating = (x: Course | Professor) => x.overall_rating ?? 0; | |
| const byDifficulty = (x: Course | Professor) => x.difficulty_rating ?? 0; | |
| const byReviews = (x: Course | Professor) => | |
| (x as Course & { review_count?: number }).review_count ?? 0; | |
| if (sortBy) { | |
| result = [...result].sort((a, b) => { | |
| switch (sortBy) { | |
| case "best-rated": | |
| return byRating(b) - byRating(a); | |
| case "most-reviewed": | |
| return byReviews(b) - byReviews(a); | |
| case "easiest": | |
| return byDifficulty(a) - byDifficulty(b); | |
| case "hardest": | |
| return byDifficulty(b) - byDifficulty(a); | |
| default: | |
| return 0; | |
| } | |
| }); | |
| } | |
| // Special case: when multiple departments are selected → sort by click order | |
| if (type === "course" && filters.departments.length > 1) { | |
| result = result.toSorted((a, b) => { | |
| const idA = departmentProperties.find((d) => d.name === (a as Course).department)?.id; | |
| const idB = departmentProperties.find((d) => d.name === (b as Course).department)?.id; | |
| const idxA = idA ? filters.departments.indexOf(idA) : -1; | |
| const idxB = idB ? filters.departments.indexOf(idB) : -1; | |
| if (idxA === -1 && idxB === -1) return 0; | |
| if (idxA === -1) return 1; | |
| if (idxB === -1) return -1; | |
| return idxA - idxB; | |
| }); | |
| } | |
| return result; | |
| }, [itemsWithAvg, filters, type, sortBy]); |
🤖 Prompt for AI Agents
In `@src/components/courses/ItemList.tsx` around lines 165 - 181, The sort
dropdown isn't applied because sorting is not performed inside the memo that
produces filteredItems and sortBy is not included in that memo's dependency
list; update the useMemo that returns filteredItems to apply sorting based on
the sortBy value (handle cases like review_count → your actual review-count
field) before returning result, include sortBy in the dependency array, and
ensure the special-case department ordering logic (currently using
departmentProperties and Course casting) remains part of that post-filter sort
pipeline so filteredItems reflects the selected sort option.


Fix Professor Filters
What changed
Issue
Fixes #21
issues link : #21
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.