Skip to content

Conversation

@nygrenh
Copy link
Member

@nygrenh nygrenh commented Nov 11, 2025

Summary by CodeRabbit

  • New Features

    • Popular course recommendations added to the front page, delivering up to four curated, language-aware suggestions and hiding ended/hidden items.
  • Tests

    • New tests verifying language filtering, exclusion of ended/hidden courses, translation accuracy, result limits, and empty-language handling for recommendations.

✏️ Tip: You can customize this high-level summary in your review settings.

@codecov
Copy link

codecov bot commented Nov 11, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.80%. Comparing base (7334d72) to head (6b56c72).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1289      +/-   ##
==========================================
+ Coverage   62.83%   65.80%   +2.97%     
==========================================
  Files         151      154       +3     
  Lines        6388     6604     +216     
  Branches     1580     1631      +51     
==========================================
+ Hits         4014     4346     +332     
+ Misses       2204     2108      -96     
+ Partials      170      150      -20     
Flag Coverage Δ
backend 65.80% <ø> (+2.97%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nygrenh nygrenh force-pushed the popular-courses-fixes branch from aca7613 to c7bf4f6 Compare November 11, 2025 10:28
@coderabbitai
Copy link

coderabbitai bot commented Jan 7, 2026

📝 Walkthrough

Walkthrough

Adds a new popularCourses GraphQL query that filters out ended/hidden courses, requires translations for a requested language, shuffles results, returns up to four items, adds backend tests for this behavior, and updates the front-end to query and render these popular courses.

Changes

Cohort / File(s) Summary
Backend GraphQL Schema
backend/schema/Course/queries.ts
Added popularCourses field to CourseQueries. Filters non-hidden, non-ended courses with null completions_handled_by_id, requires translations in the requested language, maps translation fields (name, description, link), shuffles results, and limits to 4. Imports updated to include shuffle from lodash and CourseStatus from Prisma.
Backend Tests
backend/schema/Course/__test__/Course.queries.test.ts
New popularCourses test suite: seeds data, marks one course as Ended, adds translations (e.g., fi_FI), and verifies language filtering, exclusion of ended/hidden courses, translation presence, result cap of 4, empty-language behavior, and translated field correctness.
Frontend GraphQL Query Definition
frontend/graphql/queries/course.queries.graphql
Added PopularCourses($popularCoursesForLanguage: String!, $language: String) query that calls popularCourses(popularCoursesForLanguage: $popularCoursesForLanguage) and returns ...NewCourseFields.
Frontend Component Update
frontend/components/NewLayout/Frontpage/SelectedCourses.tsx
Switched from NewCoursesDocument to PopularCoursesDocument; changed variables to { popularCoursesForLanguage: language, language }; now consumes data.popularCourses directly and removed client-side "notEndedCourses" filtering; adjusted imports accordingly.
Package Manifest
package.json
(Referenced in summary) No API/exports changed; ensure dev/test dependencies align with new tests (no explicit code changes listed).

Sequence Diagram

sequenceDiagram
    participant Client as Frontend Client
    participant GraphQL as GraphQL API
    participant Resolver as popularCourses Resolver
    participant DB as Database (Prisma)

    Client->>GraphQL: PopularCourses Query (language: "fi_FI")
    GraphQL->>Resolver: Execute popularCourses Field
    Resolver->>DB: Fetch courses where<br/>- hidden = false<br/>- status != CourseStatus.Ended<br/>- completions_handled_by_id = null
    DB-->>Resolver: Course records
    Resolver->>DB: Fetch translations for courses filtered by language
    DB-->>Resolver: Translation records

    rect rgba(100,150,200,0.5)
        Note over Resolver: Map translations into course fields
        Resolver->>Resolver: Fill name/description/link from translation (or fallback)
        Resolver->>Resolver: Shuffle results
        Resolver->>Resolver: Limit to 4 items
    end

    Resolver-->>GraphQL: Filtered & shuffled course list (max 4)
    GraphQL-->>Client: popularCourses response (id, slug, name, description, link, status)
    Client->>Client: Render SelectedCourses with popular courses
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 a rabbit hops in to celebrate

Four courses shuffled, fresh and bright,
Translated names that fit just right,
Hidden gone and endings swept,
Seeded tests ensure no step is left! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: filtering popular courses by the user's current language. This is the core modification across backend and frontend changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (2)
backend/schema/Course/__test__/Course.queries.test.ts (1)

383-393: Consider improving test isolation.

The test "excludes hidden courses" checks that the "handled" course is excluded, but this course is filtered out for two reasons:

  1. hidden: true (from fixtures)
  2. completions_handled_by_id is set (from fixtures)

The backend filters on both conditions (lines 329-334 and 349 in queries.ts), so while the test passes correctly, it's ambiguous which filter is actually being verified. Consider adding a test case with a course that is hidden but does NOT have completions_handled_by_id set to explicitly verify the hidden filter in isolation.

backend/schema/Course/queries.ts (1)

311-389: Consider extracting the shared translation mapping logic.

The implementation is functionally correct and properly filters courses by language, visibility, status, and completion handling. However, the translation mapping logic (lines 358-377) is nearly identical to the same logic in the courses query (lines 278-299).

Consider extracting this into a shared helper function to improve maintainability and reduce duplication:

const mapCourseWithTranslation = (
  course: Course & { course_translations?: Array<CourseTranslation> },
  language?: string
) => {
  const translationForLanguage = language
    ? course?.course_translations?.find((t) => t.language === language)
    : undefined
  const translation =
    translationForLanguage ?? course?.course_translations?.[0]

  return {
    ...omit(course, ["course_translations", "tags", "handles_completions_for"]),
    description: translation?.description ?? "",
    link: translation?.link ?? "",
    name: translation?.name ?? course?.name ?? "",
  }
}

This would make both resolvers more maintainable and reduce the risk of inconsistencies when updating translation logic in the future.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ffd3a1b and 89adde6.

📒 Files selected for processing (4)
  • backend/schema/Course/__test__/Course.queries.test.ts
  • backend/schema/Course/queries.ts
  • frontend/components/NewLayout/Frontpage/SelectedCourses.tsx
  • frontend/graphql/queries/course.queries.graphql
🧰 Additional context used
🧬 Code graph analysis (2)
backend/schema/Course/__test__/Course.queries.test.ts (1)
backend/tests/data/seed.ts (1)
  • seed (36-146)
backend/schema/Course/queries.ts (2)
backend/tests/data/fixtures.ts (1)
  • courses (169-324)
backend/schema/Course/model.ts (1)
  • Course (9-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: build-deploy
🔇 Additional comments (5)
frontend/graphql/queries/course.queries.graphql (1)

21-25: LGTM!

The new PopularCourses query is well-structured and correctly uses the NewCourseFields fragment for consistency with other queries.

backend/schema/Course/__test__/Course.queries.test.ts (2)

326-369: LGTM!

The test suite setup and initial tests provide good coverage of the language filtering and exclusion logic for the new popularCourses field.


651-663: LGTM!

The test query correctly mirrors the fields needed to verify the popularCourses resolver behavior.

backend/schema/Course/queries.ts (1)

1-1: LGTM!

The new imports for shuffle and CourseStatus are correctly added to support the popularCourses implementation.

Also applies to: 12-12

frontend/components/NewLayout/Frontpage/SelectedCourses.tsx (1)

2-2: LGTM!

The component correctly migrates from the NewCoursesDocument query with client-side filtering to the new PopularCoursesDocument query with server-side filtering. The changes properly update:

  • The GraphQL query and variable naming
  • The data binding to use data.popularCourses
  • Removal of the unused sample import

The loading behavior remains consistent with 4 skeleton cards.

Also applies to: 29-29, 107-112

@Redande Redande merged commit e15d8b9 into master Jan 23, 2026
7 checks passed
@Redande Redande deleted the popular-courses-fixes branch January 23, 2026 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants