Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { Button } from '@/components/ui/button'
import {
Card,
CardContent,
CardDescription,
CardFooter,
CardHeader,
CardTitle,
} from '@/components/ui/card'
import { CourseDetail, Module } from '@/data/course'
import Link from 'next/link'

type Props = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: Generic Props type should be renamed to ModuleCardProps for better searchability, IDE support, and maintainability. Follows industry standards and makes refactoring easier.

// ❌ Current
type Props = { course: CourseDetail; module: Module }

// ✅ Suggested  
type ModuleCardProps = { course: CourseDetail; module: Module }

course: CourseDetail
module: Module
}

export default function ModuleCard({ course, module }: Props) {
const coursePageUrl = `/${course.sourceLanguage.code}/courses/${course.targetLanguage.code}/courses/${module.slug}`
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug: Potential Runtime Crash in ModuleCard Component

Issue: The URL building logic in ModuleCard can crash if course or module objects have null/undefined nested properties.

Current problematic code:

const coursePageUrl = `/${course.sourceLanguage.code}/courses/${course.targetLanguage.code}/courses/${module.slug}`

If course or module is null or undefined, then the app might crash 


return (
<Card>
<CardHeader>
<CardTitle>{module.title}</CardTitle>
<CardDescription>Card Description</CardDescription>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<CardDescription>Card Description</CardDescription>
<CardDescription>{module.description}</CardDescription>

</CardHeader>
<CardContent>
<p>Card Content</p>
</CardContent>
<CardFooter>
<Button asChild>
<Link href={coursePageUrl}>Learn</Link>
</Button>
</CardFooter>
</Card>
)
}
Original file line number Diff line number Diff line change
@@ -1,24 +1,40 @@
import { getCourseDetail, getCourseId, listAvailableCourses } from "@/data/course"
import {
getCourseDetail,
getCourseId,
listAvailableCourses,
} from '@/data/course'
import ModuleCard from './module-card'

export async function generateStaticParams() {
const courses = await listAvailableCourses()
const courses = await listAvailableCourses()

return courses.map((course) => ({
sourceLanguageCode: course.uiLanguage,
targetLanguageCode: course.languageCode,
}))
return courses.map((course) => ({
sourceLanguageCode: course.uiLanguage,
targetLanguageCode: course.languageCode,
}))
}

type Props = {
params: {
sourceLanguageCode: string
targetLanguageCode: string
}
params: {
sourceLanguageCode: string
targetLanguageCode: string
}
}

export default async function CourseHomePage({params}: Props) {
const courseId = await getCourseId(params)
const detail = await getCourseDetail(courseId)
export default async function CourseHomePage({ params }: Props) {
const courseId = await getCourseId(params)
const detail = await getCourseDetail(courseId)

return <h1>{detail.targetLanguage.name}</h1>
return (
<>
<h1>{detail.targetLanguage.name}</h1>
<ul className="flex space-y-6 flex-col p-6">
{detail.modules.map((module) => (
<li key={module.slug}>
<ModuleCard course={detail} module={module} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Pass coursePageUrl from parent instead of building it inside the component. Keeps the card as a pure display component without business logic.

// ✅ Better - Pure display component
type ModuleCardProps = {
 course: CourseDetail
 module: Module
 coursePageUrl: string  // Parent handles URL logic
}

</li>
))}
</ul>
</>
)
}
1 change: 1 addition & 0 deletions apps/librelingo-web/src/courses/test-1/courseData.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
},
"modules": [
{
"slug": "basics",
"title": "Basics",
"skills": [
{
Expand Down
35 changes: 26 additions & 9 deletions apps/librelingo-web/src/data/course.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import path from 'node:path'
import courseConfig from '@/courses/config.json'
import fs from 'node:fs'
import { notFound } from 'next/navigation'
import { getAbsoluteCoursePath } from './utils'

export type CourseIdentityDescription = {
sourceLanguageCode: string
Expand All @@ -20,13 +21,7 @@ export type Course = {
}

function getFullJsonPath(jsonPath: string) {
return path.join(
process.cwd(),
'src',
'courses',
jsonPath,
'courseData.json'
)
return path.join(getAbsoluteCoursePath(jsonPath), 'courseData.json')
}

async function getCourseMetadataByJsonPath(jsonPath: string) {
Expand Down Expand Up @@ -86,12 +81,34 @@ export async function getCourseId(
return course.id
}

export async function getCourseDetail(courseId: string) {
const { languageName } = await getCourseMetadataByJsonPath(courseId)
export type Module = {
title: string
slug: string
}

export type CourseDetail = {
targetLanguage: {
name: string
code: string
}
sourceLanguage: {
code: string
}
modules: Module[]
}

export async function getCourseDetail(courseId: string): Promise<CourseDetail> {
const { languageName, modules, uiLanguage, languageCode } =
await getCourseMetadataByJsonPath(courseId)

return {
targetLanguage: {
name: languageName,
code: languageCode,
},
sourceLanguage: {
code: uiLanguage
},
modules,
}
}
5 changes: 5 additions & 0 deletions apps/librelingo-web/src/data/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import path from 'node:path'

export function getAbsoluteCoursePath(jsonPath: string) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Use default export since the file contains only a single function.

// ✅ Better - Default export for single function module
export default function getAbsoluteCoursePath(jsonPath: string) {
 return path.join(process.cwd(), 'src', 'courses', jsonPath)
}

return path.join(process.cwd(), 'src', 'courses', jsonPath)
}
23 changes: 23 additions & 0 deletions e2e-tests/course.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,26 @@ test('has the correct content', async ({ page }) => {
page.getByRole('heading', { name: 'Test Language' })
).toBeVisible()
})

test('has cards leading to module pages', async ({ page }) => {
const courseHomePagePattern = new RegExp(
`[^/]*/courses/[^/]+/modules/[^/]+`
)
await page.goto('/en/courses/test-1')

const cards = await page.getByRole('listitem').all()

expect(cards.length).toBeGreaterThanOrEqual(1)
const urls = new Set()

for (const card of cards) {
const button = card.getByRole('link', { name: 'Learn' })
const url = await button.getAttribute('href')

expect(url).toMatch(courseHomePagePattern)
urls.add(url)
}

// each course has to have a unique url
expect(urls.size).toBeGreaterThanOrEqual(cards.length)
})
4 changes: 3 additions & 1 deletion e2e-tests/home.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ test('has the correct content', async ({ page }) => {
await expect(firstCard.getByRole('link', { name: 'Learn' })).toBeVisible()
})

test('all card buttons lead to URLs matching the pattern', async ({ page }) => {
test('has cards for each course leading to the course page', async ({
page,
}) => {
const courseHomePagePattern = new RegExp(`[^/]*/courses/[^/]+`)
await page.goto('/')

Expand Down
1 change: 1 addition & 0 deletions src/librelingo_json_export/module.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ def get_levels(words, phrases):
return calculate_number_of_levels(len(words), len(phrases))

return {
"slug": slugify(module.title),
"title": module.title,
"skills": [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ def test__get_course_data_return_value():
},
"modules": [
{
"slug": "basics",
"title": "Basics",
"skills": [
{
Expand Down Expand Up @@ -67,7 +68,7 @@ def test__get_course_data_return_value():
},
],
},
{"title": "Phrases", "skills": []},
{"slug": "phrases", "title": "Phrases", "skills": []},
],
}

Expand All @@ -89,6 +90,7 @@ def test__get_course_data_return_value_2():
},
"modules": [
{
"slug": "animals",
"title": "Animals",
"skills": [
{
Expand Down Expand Up @@ -122,6 +124,7 @@ def test__get_course_data_return_value_with_introduction():
},
"modules": [
{
"slug": "animals",
"title": "Animals",
"skills": [
{
Expand Down