[SPRINT-M25] Implement Top Skills section with API integration and custom hook #193
Conversation
|
@aviralsaxena16 is attempting to deploy a commit to the openlake's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes introduce a "top skills" feature by extending the backend skills API with new endpoints for endorsing individual user skills and retrieving the top 5 most popular skills campus-wide. Simultaneously, the frontend implements a React hook and UI component to fetch and display these top skills with appropriate loading and empty states. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as TopSkills Component
participant Hook as useTopSkills Hook
participant API as Backend API
participant DB as Database
User->>Frontend: Mount TopSkills component
Frontend->>Hook: Initialize hook
Hook->>API: GET /api/skills/top-skills
activate API
API->>DB: Aggregate top 5 popular skills
DB-->>API: Return aggregated skills
API-->>Hook: Response with top skills data
deactivate API
Hook-->>Frontend: { topSkills, loading: false }
Frontend->>Frontend: Render skill list
Frontend-->>User: Display top 5 skills with count
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/src/hooks/useTopSkills.js (1)
1-23: Consider exposing error state for better UX.The hook logs errors to the console but doesn't expose an error state to the consuming component. This makes it impossible for the UI to distinguish between "no data available" and "fetch failed," preventing users from seeing appropriate error messages or retry options.
Apply this diff to add error state handling:
export const useTopSkills = () => { const [topSkills, setTopSkills] = useState([]); const [loading, setLoading] = useState(true); + const [error, setError] = useState(null); useEffect(() => { const fetchTopSkills = async () => { try { const res = await api.get("/api/skills/top-skills"); setTopSkills(res.data); + setError(null); } catch (error) { console.error("Error fetching top skills:", error); + setError(error); } finally { setLoading(false); } }; fetchTopSkills(); }, []); - return { topSkills, loading }; + return { topSkills, loading, error }; };frontend/src/Components/Skills/TopSkills.jsx (2)
16-25: Add error state handling when hook is updated.The component currently only handles loading and empty states. If the useTopSkills hook is updated to expose an error state (as suggested in the hook review), this component should display an appropriate error message to inform users when the fetch fails.
When the hook is updated, add error handling:
const TopSkills = () => { - const { topSkills, loading } = useTopSkills(); + const { topSkills, loading, error } = useTopSkills(); return ( <div className="px-6 pt-6 pb-3 flex flex-col items-start justify-between flex-wrap gap-3 w-full h-full overflow-y-auto"> {/* Header */} <div className="text-2xl font-bold tracking-tight text-gray-900"> Top 5 Skills </div> {/* Loader / Empty / List */} {loading ? ( <div className="flex justify-center items-center flex-1 w-full"> <Loader2 className="w-5 h-5 animate-spin text-gray-400" /> </div> + ) : error ? ( + <div className="flex justify-center items-center flex-1 w-full"> + <p className="text-red-500 text-sm text-center"> + Failed to load top skills. Please try again later. + </p> + </div> ) : topSkills.length === 0 ? (
27-46: Remove redundant slice operation.Line 28 slices the array to 5 items, but the backend API endpoint already limits results to 5 (see Line 170 in backend/routes/skillsRoutes.js:
{ $limit: 5 }). This slice is redundant and can be removed for cleaner code.Apply this diff:
- <ul className="flex flex-col gap-0.5 flex-1 overflow-y-auto w-full -ml-1"> - {topSkills.slice(0, 5).map((skill, index) => ( + <ul className="flex flex-col gap-0.5 flex-1 overflow-y-auto w-full -ml-1"> + {topSkills.map((skill, index) => ( <liNote: Using
indexas a key (Line 30) is acceptable here since the list is small, static, and items don't have unique IDs in the API response.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/routes/skillsRoutes.js(6 hunks)frontend/src/Components/Skills/TopSkills.jsx(1 hunks)frontend/src/hooks/useTopSkills.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/src/hooks/useTopSkills.js (3)
frontend/src/Components/Skills/TopSkills.jsx (1)
useTopSkills(6-6)backend/routes/skillsRoutes.js (1)
topSkills(167-188)frontend/src/utils/api.js (1)
api(2-5)
backend/routes/skillsRoutes.js (3)
backend/models/schema.js (2)
UserSkill(637-637)Skill(638-638)backend/utils/roles.js (1)
ROLE_GROUPS(11-29)backend/seed.js (1)
console(373-406)
frontend/src/Components/Skills/TopSkills.jsx (2)
frontend/src/hooks/useTopSkills.js (4)
useTopSkills(4-23)useTopSkills(4-23)loading(6-6)topSkills(5-5)backend/routes/skillsRoutes.js (3)
topSkills(167-188)skill(78-78)skill(130-136)
🔇 Additional comments (3)
backend/routes/skillsRoutes.js (3)
9-33: LGTM! Clean formatting and proper middleware usage.The reformatted endpoint maintains the original logic with improved readability. The use of
isAuthenticatedmiddleware and proper error handling is appropriate.
35-54: LGTM! Proper authorization and error handling.The new endpoint correctly restricts endorsement to admin roles and handles both success and error cases appropriately.
70-93: LGTM! Enhanced response provides more context.The reformatted endpoint now returns the endorsed skill in the response, which is a useful improvement for client-side updates.
| // GET top 5 most popular skills campus-wide | ||
| router.get("/top-skills", isAuthenticated, async (req, res) => { | ||
| try { | ||
| const topSkills = await UserSkill.aggregate([ | ||
| { $group: { _id: "$skill_id", totalUsers: { $sum: 1 } } }, | ||
| { $sort: { totalUsers: -1 } }, | ||
| { $limit: 5 }, | ||
| { | ||
| $lookup: { | ||
| from: "skills", | ||
| localField: "_id", | ||
| foreignField: "_id", | ||
| as: "skillDetails", | ||
| }, | ||
| }, | ||
| { $unwind: "$skillDetails" }, | ||
| { | ||
| $project: { | ||
| _id: 0, | ||
| skillName: "$skillDetails.name", | ||
| type: "$skillDetails.type", | ||
| totalUsers: 1, | ||
| }, | ||
| }, | ||
| ]); | ||
|
|
||
| res.json(topSkills); | ||
| } catch (err) { | ||
| console.error(err); | ||
| res.status(500).json({ message: "Error fetching top skills." }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Filter for endorsed skills to ensure data quality.
The aggregation pipeline counts all UserSkills without filtering by endorsement status. Based on the codebase context, skills follow an endorsement workflow (see lines 16, 43, 61, 84, 98), but this endpoint includes unendorsed entries in the counts.
This could inflate user counts and surface skills that haven't been validated, which is inconsistent with the endorsement model used throughout the codebase.
Apply this diff to filter for endorsed UserSkills:
router.get("/top-skills", isAuthenticated, async (req, res) => {
try {
const topSkills = await UserSkill.aggregate([
+ { $match: { is_endorsed: true } },
{ $group: { _id: "$skill_id", totalUsers: { $sum: 1 } } },
{ $sort: { totalUsers: -1 } },
{ $limit: 5 },Additionally, consider filtering for endorsed skills in the lookup to ensure skill details are also validated:
{
$lookup: {
from: "skills",
localField: "_id",
foreignField: "_id",
as: "skillDetails",
+ pipeline: [{ $match: { is_endorsed: true } }],
},
},📝 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.
| // GET top 5 most popular skills campus-wide | |
| router.get("/top-skills", isAuthenticated, async (req, res) => { | |
| try { | |
| const topSkills = await UserSkill.aggregate([ | |
| { $group: { _id: "$skill_id", totalUsers: { $sum: 1 } } }, | |
| { $sort: { totalUsers: -1 } }, | |
| { $limit: 5 }, | |
| { | |
| $lookup: { | |
| from: "skills", | |
| localField: "_id", | |
| foreignField: "_id", | |
| as: "skillDetails", | |
| }, | |
| }, | |
| { $unwind: "$skillDetails" }, | |
| { | |
| $project: { | |
| _id: 0, | |
| skillName: "$skillDetails.name", | |
| type: "$skillDetails.type", | |
| totalUsers: 1, | |
| }, | |
| }, | |
| ]); | |
| res.json(topSkills); | |
| } catch (err) { | |
| console.error(err); | |
| res.status(500).json({ message: "Error fetching top skills." }); | |
| } | |
| }); | |
| // GET top 5 most popular skills campus-wide | |
| router.get("/top-skills", isAuthenticated, async (req, res) => { | |
| try { | |
| const topSkills = await UserSkill.aggregate([ | |
| { $match: { is_endorsed: true } }, | |
| { $group: { _id: "$skill_id", totalUsers: { $sum: 1 } } }, | |
| { $sort: { totalUsers: -1 } }, | |
| { $limit: 5 }, | |
| { | |
| $lookup: { | |
| from: "skills", | |
| localField: "_id", | |
| foreignField: "_id", | |
| as: "skillDetails", | |
| }, | |
| }, | |
| { $unwind: "$skillDetails" }, | |
| { | |
| $project: { | |
| _id: 0, | |
| skillName: "$skillDetails.name", | |
| type: "$skillDetails.type", | |
| totalUsers: 1, | |
| }, | |
| }, | |
| ]); | |
| res.json(topSkills); | |
| } catch (err) { | |
| console.error(err); | |
| res.status(500).json({ message: "Error fetching top skills." }); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In backend/routes/skillsRoutes.js around lines 164 to 195, the aggregation
currently counts all UserSkill documents but should only include endorsed ones;
add a $match stage early in the pipeline to filter UserSkill documents where the
endorsement flag/status (e.g., endorsed: true or status: "endorsed" — use the
canonical field used elsewhere in the codebase) is satisfied, then continue
grouping/sorting/limiting; additionally, tighten the $lookup by matching only
skills that are validated/endorsed (if the skills collection has a validation
flag) — either add a pipeline form of $lookup with a $match on skill validation
or filter the joined results after $lookup to ensure only validated skillDetails
are returned, and handle cases where a skillDetails entry may be missing by
adjusting the $unwind/$match accordingly.
There was a problem hiding this comment.
@aviralsaxena16 consider this and make changes accordingly, as only endorsed skills must be counted.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@aviralsaxena16 I have left comments, please update the code accordingly |
2 participants
Implemented the Top Skills segment by creating a custom hook useTopSkills, adding a new API endpoint in the skills route, and developing the TopSkills.jsx component (previously blank) to display the top five most popular skills dynamically with proper loading and empty states.
Close: #178
Review: @harshitap1305