Fix/2505/guided learning paths for dsa learners#2623
Conversation
…nto fix/2505/Guided-Learning-Paths-for-DSA-Learners
|
Thank you for submitting your pull request, @trushi-jasani! 🙌 We'll review it as soon as possible. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution to our Algo project! 😊 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive 'Guided Learning Paths' feature, adding structured learning paths, metadata-rich topics, interactive components (PathCard, TopicCard), and dedicated hub and detail pages, along with extensive documentation. The review feedback highlights critical issues that must be addressed before merging: a potential SSR build crash from direct window object access, a React state anti-pattern on dynamic routes that prevents content updates, and full page reloads caused by using window.location.href for internal navigation. Additionally, the reviewer pointed out duplicate JSX props, logical inconsistencies in progress tracking and topic ordering, unused imports, and opportunities to reduce code duplication by reusing the TopicCard component.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const searchParams = new URLSearchParams(location?.search || window.location.search); | ||
| const pathId = searchParams.get("id") || "beginner-programmer"; |
There was a problem hiding this comment.
Accessing window.location.search directly during the render phase will throw a ReferenceError: window is not defined during Docusaurus static site generation (SSR/SSG) and crash the production build.
Use useLocation from @docusaurus/router to safely access the search query in an SSR-compatible way (make sure to import useLocation from @docusaurus/router).
| const searchParams = new URLSearchParams(location?.search || window.location.search); | |
| const pathId = searchParams.get("id") || "beginner-programmer"; | |
| const location = useLocation(); | |
| const searchParams = useMemo(() => new URLSearchParams(location.search), [location.search]); | |
| const pathId = searchParams.get("id") || "beginner-programmer"; |
| const [path, setPath] = useState(() => { | ||
| if (typeof window !== "undefined") { | ||
| const urlPath = window.location.pathname.split("/").pop(); | ||
| return getLearningPathById(urlPath || pathId || ""); | ||
| } | ||
| return getLearningPathById(pathId || ""); | ||
| }); |
There was a problem hiding this comment.
Storing the path in useState with an initializer function is a React anti-pattern for dynamic routes. When a user navigates between different learning paths (e.g., from /learning-paths/beginner-programmer to /learning-paths/dsa-fundamentals), React reuses the component instance, meaning the state initializer will not run again and the page will display the old path's content.
Additionally, setPath is never used. Instead, derive the path value directly during render using useParams from @docusaurus/router and useMemo (make sure to import useParams from @docusaurus/router).
const { pathId: routePathId } = useParams<{ pathId: string }>();
const activePathId = pathId || routePathId;
const path = useMemo(() => getLearningPathById(activePathId || ""), [activePathId]);
| onExplore={(pathId) => { | ||
| window.location.href = `/algo/learning-paths/path-details?id=${pathId}`; | ||
| }} |
There was a problem hiding this comment.
Using window.location.href or event handlers for internal navigation causes a full page reload, which destroys the application state and degrades user experience. This violates the repository's rule.
Instead of using the onExplore event handler, refactor the code to use the Link component from @docusaurus/Link to wrap the card or within the card to handle navigation smoothly.
// Wrap the PathCard component with Link from @docusaurus/Link instead of using onExplore
References
- For internal navigation in Docusaurus or React applications, use the Link component instead of window.location.href or onClick event handlers to prevent full page reloads and maintain application state.
| import { | ||
| FaArrowLeft, | ||
| FaCheckCircle, | ||
| FaClock, | ||
| FaTrophy, | ||
| FaBook, | ||
| FaChartLine, | ||
| } from "react-icons/fa"; |
There was a problem hiding this comment.
Several icons imported from react-icons/fa are not used in this file (FaCheckCircle, FaClock, FaTrophy, FaBook, FaChartLine). Removing unused imports keeps the codebase clean and avoids unnecessary bundle size overhead.
import {
FaArrowLeft,
} from "react-icons/fa";
| calculateTotalTime, | ||
| getLearningPathById, | ||
| } from "../../data/learningPaths"; | ||
| import TopicCard from "../../components/LearningPaths/TopicCard"; |
There was a problem hiding this comment.
Since you created a centralized index file (src/components/LearningPaths/index.ts) to export the components, you should use named imports from the folder instead of importing the default export from the individual file directly.
| import TopicCard from "../../components/LearningPaths/TopicCard"; | |
| import { TopicCard } from "../../components/LearningPaths"; |
| FaArrowLeft, | ||
| FaClock, | ||
| FaTrophy, | ||
| FaBook, | ||
| FaCheckCircle, | ||
| FaPlayCircle, | ||
| FaExternalLinkAlt, | ||
| } from "react-icons/fa"; |
There was a problem hiding this comment.
The icons FaPlayCircle and FaExternalLinkAlt are imported but never used in this file. Removing them keeps the imports clean.
import {
FaArrowLeft,
FaClock,
FaTrophy,
FaBook,
FaCheckCircle,
} from "react-icons/fa";
| const toggleInProgress = (topicId: string) => { | ||
| const newSet = new Set(inProgressTopics); | ||
| if (newSet.has(topicId)) { | ||
| newSet.delete(topicId); | ||
| } else { | ||
| newSet.add(topicId); | ||
| } | ||
| setInProgressTopics(newSet); | ||
| }; |
There was a problem hiding this comment.
In toggleInProgress, when a topic is marked as in progress, it is not removed from completedTopics. This allows a topic to be in both completedTopics and inProgressTopics simultaneously, which is logically inconsistent.
Update the function to remove the topic from completedTopics when marking it as in progress, matching the logic in [pathId].tsx.
const toggleInProgress = (topicId: string) => {
const newSet = new Set(inProgressTopics);
if (newSet.has(topicId)) {
newSet.delete(topicId);
} else {
newSet.add(topicId);
setCompletedTopics((prev) => {
const updated = new Set(prev);
updated.delete(topicId);
return updated;
});
}
setInProgressTopics(newSet);
};
| const isCompleted = completedTopics.has(topic.id); | ||
| const isInProgress = inProgressTopics.has(topic.id); | ||
|
|
||
| return ( | ||
| <motion.div | ||
| key={topic.id} | ||
| initial={{ opacity: 0, x: -20 }} | ||
| animate={{ opacity: 1, x: 0 }} | ||
| transition={{ delay: idx * 0.05 }} | ||
| className={`bg-white dark:bg-gray-800 rounded-lg shadow-md p-6 border-l-4 transition-all ${ | ||
| isCompleted | ||
| ? "border-l-green-500 bg-green-50 dark:bg-gray-800/50" | ||
| : isInProgress | ||
| ? "border-l-blue-500 bg-blue-50 dark:bg-gray-800/50" | ||
| : "border-l-gray-300 dark:border-l-gray-600" | ||
| }`} | ||
| > | ||
| <div className="flex items-start justify-between"> | ||
| <div className="flex-1"> | ||
| <div className="flex items-center gap-3 mb-2"> | ||
| <span className="text-2xl">{topic.icon}</span> | ||
| <h4 className="text-lg font-semibold text-gray-900 dark:text-white"> | ||
| {topic.title} | ||
| </h4> | ||
| </div> | ||
| <p className="text-sm text-gray-600 dark:text-gray-400 mb-3"> | ||
| {topic.description} | ||
| </p> | ||
|
|
||
| {/* Topic Metadata */} | ||
| <div className="grid grid-cols-3 sm:grid-cols-4 gap-3 text-xs mb-3"> | ||
| {/* Difficulty */} | ||
| <div> | ||
| <span className="text-gray-500 dark:text-gray-400">Difficulty</span> | ||
| <div | ||
| className={`mt-1 inline-block px-2 py-1 rounded-full text-xs font-medium ${ | ||
| topic.difficulty === "Easy" | ||
| ? "bg-green-100 text-green-800 dark:bg-green-900/30 dark:text-green-300" | ||
| : topic.difficulty === "Medium" | ||
| ? "bg-yellow-100 text-yellow-800 dark:bg-yellow-900/30 dark:text-yellow-300" | ||
| : "bg-red-100 text-red-800 dark:bg-red-900/30 dark:text-red-300" | ||
| }`} | ||
| > | ||
| {topic.difficulty} | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Time */} | ||
| <div> | ||
| <span className="text-gray-500 dark:text-gray-400">Time</span> | ||
| <div className="mt-1 font-medium text-gray-900 dark:text-white"> | ||
| {topic.estimatedTime}h | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Interview Relevance */} | ||
| <div> | ||
| <span className="text-gray-500 dark:text-gray-400">Relevance</span> | ||
| <div className="mt-1 text-yellow-500">{getRelevanceStars(topic.interviewRelevance)}</div> | ||
| </div> | ||
|
|
||
| {/* Prerequisites */} | ||
| {topic.prerequisites.length > 0 && ( | ||
| <div> | ||
| <span className="text-gray-500 dark:text-gray-400">Prerequisites</span> | ||
| <div className="mt-1 font-medium text-gray-900 dark:text-white"> | ||
| {topic.prerequisites.length} | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Prerequisites List */} | ||
| {topic.prerequisites.length > 0 && ( | ||
| <div className="mt-3 p-3 bg-gray-100 dark:bg-gray-700 rounded text-sm"> | ||
| <p className="font-medium text-gray-700 dark:text-gray-300 mb-2"> | ||
| Prerequisites: | ||
| </p> | ||
| <div className="flex flex-wrap gap-2"> | ||
| {topic.prerequisites.map((prereqId) => { | ||
| const prereq = path.topics.find((t) => t.id === prereqId); | ||
| return ( | ||
| <span | ||
| key={prereqId} | ||
| className="inline-block bg-blue-100 text-blue-800 dark:bg-blue-900/30 dark:text-blue-300 px-2 py-1 rounded text-xs" | ||
| > | ||
| {prereq?.title || prereqId} | ||
| </span> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
|
|
||
| {/* Action Buttons */} | ||
| <div className="ml-4 flex flex-col gap-2"> | ||
| <button | ||
| onClick={() => toggleCompleted(topic.id)} | ||
| className={`px-3 py-2 rounded-lg text-xs font-medium transition-all ${ | ||
| isCompleted | ||
| ? "bg-green-600 text-white" | ||
| : "bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300 hover:bg-green-200 dark:hover:bg-green-900" | ||
| }`} | ||
| title="Mark as Completed" | ||
| > | ||
| {isCompleted ? "✓ Done" : "Complete"} | ||
| </button> | ||
| </div> | ||
| </div> | ||
| </motion.div> | ||
| ); | ||
| })} | ||
| </div> |
There was a problem hiding this comment.
You have duplicated the entire layout and logic of the topic card inline here instead of reusing the TopicCard component you created in src/components/LearningPaths/TopicCard.tsx.
Reusing the TopicCard component here will drastically reduce code duplication, improve maintainability, and ensure a consistent UI across the entire feature (make sure to import TopicCard from ../../components/LearningPaths).
<div className="grid gap-4 md:grid-cols-2 lg:grid-cols-1">
{filteredTopics.map((topic, idx) => (
<TopicCard
key={topic.id}
topic={topic}
index={idx}
isCompleted={completedTopics.has(topic.id)}
isInProgress={inProgressTopics.has(topic.id)}
onToggleComplete={toggleCompleted}
onToggleInProgress={toggleInProgress}
/>
))}
</div>
| <motion.div | ||
| variants={cardVariants} | ||
| initial="hidden" | ||
| whileInView="show" | ||
| viewport={{ once: true, margin: "-50px" }} | ||
| whileHover="hover" | ||
| variants={{ | ||
| ...cardVariants, | ||
| ...hoverVariants, | ||
| }} | ||
| className={`group rounded-2xl border border-gray-200 p-6 shadow-md transition-all duration-300 dark:border-gray-700 bg-gradient-to-br ${getGradientClass()} hover:shadow-xl dark:shadow-lg`} |
There was a problem hiding this comment.
The variants prop is defined twice on the motion.div component (on line 54 and again on line 59). In JSX, duplicate props are overridden by the last definition, which can lead to unexpected behavior and confusion. Clean this up by defining the combined variants once.
| <motion.div | |
| variants={cardVariants} | |
| initial="hidden" | |
| whileInView="show" | |
| viewport={{ once: true, margin: "-50px" }} | |
| whileHover="hover" | |
| variants={{ | |
| ...cardVariants, | |
| ...hoverVariants, | |
| }} | |
| className={`group rounded-2xl border border-gray-200 p-6 shadow-md transition-all duration-300 dark:border-gray-700 bg-gradient-to-br ${getGradientClass()} hover:shadow-xl dark:shadow-lg`} | |
| <motion.div | |
| initial="hidden" | |
| whileInView="show" | |
| viewport={{ once: true, margin: "-50px" }} | |
| whileHover="hover" | |
| variants={{ | |
| ...cardVariants, | |
| ...hoverVariants, | |
| }} | |
| className={`group rounded-2xl border border-gray-200 p-6 shadow-md transition-all duration-300 dark:border-gray-700 bg-gradient-to-br ${getGradientClass()} hover:shadow-xl dark:shadow-lg`} | |
| > |
| { | ||
| id: "linked-lists", | ||
| title: "Linked Lists", | ||
| difficulty: "Medium", | ||
| estimatedTime: 2.5, | ||
| prerequisites: ["pointers"], | ||
| interviewRelevance: 5, | ||
| description: | ||
| "Understand singly/doubly linked lists, operations, and common problems", | ||
| icon: "🔗", | ||
| docLink: "/docs/data-structures", | ||
| }, | ||
| { | ||
| id: "pointers", | ||
| title: "Pointers & References", | ||
| difficulty: "Medium", | ||
| estimatedTime: 1.5, | ||
| prerequisites: ["variables-data-types"], | ||
| interviewRelevance: 4, | ||
| description: | ||
| "Master pointer concepts essential for linked lists and advanced DS", | ||
| icon: "👉", | ||
| docLink: "/docs/basic-data-structures/pointer", |
There was a problem hiding this comment.
In the dsaFundamentalsTopics array, linked-lists is defined before pointers. However, linked-lists lists pointers as a prerequisite. This means a student following the natural order of the array will encounter linked-lists before they have been introduced to pointers.
Swap the order of these two topics in the array so that the prerequisite (pointers) is introduced first.
{
id: "pointers",
title: "Pointers & References",
difficulty: "Medium",
estimatedTime: 1.5,
prerequisites: ["variables-data-types"],
interviewRelevance: 4,
description:
"Master pointer concepts essential for linked lists and advanced DS",
icon: "👉",
docLink: "/docs/basic-data-structures/pointer",
},
{
id: "linked-lists",
title: "Linked Lists",
difficulty: "Medium",
estimatedTime: 2.5,
prerequisites: ["pointers"],
interviewRelevance: 5,
description:
"Understand singly/doubly linked lists, operations, and common problems",
icon: "🔗",
docLink: "/docs/data-structures",
},
⚡️ Lighthouse Report for the Deploy Preview of this PR 🚀
|
1 participant
🚀 Add Structured Learning Paths for DSA Preparation
Overview
This PR introduces a new Learning Paths feature that provides a structured roadmap for users preparing Data Structures & Algorithms. Users can choose a path based on their current skill level and learning goals, then follow curated topics with metadata-driven guidance.
✨ Features Added
PathCardcomponentTopicCardcomponent📚 Learning Paths Included
🎯 Benefits
🛠 Technical Changes
Future Scope
Screenshots
Testing
Closes #2505