Add HTTP endpoints #1302
Add HTTP endpoints #1302
Conversation
|
Warning Rate limit exceeded@nygrenh has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a backend PublicController with four /public JSON endpoints and migrates frontend data fetching from Apollo/GraphQL to a REST + React Query stack with a centralized apiClient, typed API shapes, hooks, and component updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Frontend Component
participant Hook as React Query Hook
participant Client as apiClient
participant Server as Backend (/api/public)
participant DB as Database / Loaders
rect rgb(245,250,255)
Note over UI,DB: Fetch public courses (language-aware)
UI->>Hook: call useCoursesData(language)
activate Hook
Hook->>Client: getCourses(language)
activate Client
Client->>Server: GET /api/public/courses?language=xx
activate Server
Server->>DB: query courses + invoke loaders (tags, sponsors, translations)
activate DB
DB-->>Server: rows & relations
deactivate DB
Server->>Server: enrich, translate, assemble JSON
Server-->>Client: 200 { courses: [...] }
deactivate Server
Client-->>Hook: Promise resolved with data
deactivate Client
Hook->>UI: returns { data, isLoading, error }
deactivate Hook
UI->>UI: render using data
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/components/NewLayout/Frontpage/SelectedCourses.tsx (1)
50-87: Remove dead code or track as technical debt.The
CourseCardcomponent is marked as "not used for now" with a@ts-ignoredirective. Dead code increases maintenance burden and can cause confusion. Consider:
- Remove it entirely if not needed
- If planned for future use, create an issue to track it and remove it from this PR
♻️ Proposed fix: Remove the unused component
-// @ts-ignore: not used for now -const CourseCard = ({ - name, - description, - start_date, - end_date, -}: { - name: string - description?: string | null - start_date?: string | null - end_date?: string | null -}) => { - const date = - start_date && end_date - ? `${formatDateTime(start_date)} - ${formatDateTime(end_date)}` - : "Jatkuva kurssi" - - return ( - <CardWrapper> - <CardHeader> - <CardTitle variant="h4">{name}</CardTitle> - <CardHeaderImage - alt="MOOC logo" - src={moocLogo} - width={200} - height={200} - /> - </CardHeader> - <CardBody> - <CardDescription>{description}</CardDescription> - <CardActions> - <CourseDate>{date}</CourseDate> - <Button>Kurssin tiedot</Button> - </CardActions> - </CardBody> - </CardWrapper> - ) -} -frontend/components/NewLayout/Courses/CourseGrid.tsx (1)
243-255: Bug incompareCourses: incorrect comparison operator causes unstable sorting.At line 250, using
>=instead of>means that when course names are equal, it returns1instead of0. This violates sort comparator contract (should return0for equal elements) and can cause unstable or unpredictable sorting behavior.🐛 Proposed fix
const compareCourses = (course1: NewCourseFields, course2: NewCourseFields) => { if (course1.study_modules.length == 0) { return 1 } else if (course2.study_modules.length == 0) { return -1 } else if (course1.study_modules[0].name < course2.study_modules[0].name) { return -1 - } else if (course1.study_modules[0].name >= course2.study_modules[0].name) { + } else if (course1.study_modules[0].name > course2.study_modules[0].name) { return 1 } else { return 0 } }
🤖 Fix all issues with AI agents
In @backend/api/routes/public.ts:
- Around line 65-84: The course translation/filtering and enrichment logic is
duplicated across the frontpage, courses, and studyModules endpoints; extract a
reusable helper (e.g., filterCoursesWithTranslations) that accepts courses and
optional language and returns the mapped/filtered course objects (using omit and
isDefined), and extract an async enrichCourses helper that accepts the courses
array, loaders (createLoaders return type) and prisma plus options
(includeStudyModules flag) to batch-fetch tags, sponsors, and study modules and
attach them, then replace the inline blocks in frontpage, courses, and
studyModules to call these helpers to remove duplication and centralize batched
fetching logic.
- Around line 194-231: filteredCourses is causing N+1 queries by calling
loaders.courseTags.load, loaders.courseSponsors.load,
prisma.courseStudyModule.findMany and prisma.studyModule.findMany inside the
loop; fix it by batching: (1) collect all course ids from filteredCourses, (2)
fetch all courseStudyModule rows for those ids in one
prisma.courseStudyModule.findMany call, (3) fetch all study modules with
prisma.studyModule.findMany using the collected study_module_id list and then
group modules back to each course to set course.study_modules, (4) replace
per-course loaders.courseTags.load and loaders.courseSponsors.load calls with
batched loader methods or single Prisma queries that return tags and sponsors
for all course ids and then map them to each course, and (5) move the enrichment
logic (tags, sponsors, study_modules, course_translations mapping) into a shared
helper function used by frontpage, courses and studyModules to eliminate
duplication (reference filteredCourses, loaders.courseTags.load,
loaders.courseSponsors.load, prisma.courseStudyModule.findMany,
prisma.studyModule.findMany, course.study_modules, course.course_translations).
- Around line 95-126: The loop over filteredCourses causes N+1 DB calls because
prisma.courseStudyModule.findMany and prisma.studyModule.findMany are called
per-course; instead, batch-fetch all courseStudyModule rows for all course IDs
(use filteredCourses.map(c => c.id)) in one query, collect unique
study_module_id values, fetch all study modules in one
prisma.studyModule.findMany by those IDs, build a lookup map from
study_module_id to module, then for each course assign course.study_modules by
mapping its related study_module_ids to modules using that lookup; keep the
existing loaders for tags/sponsors as-is and remove the per-course Prisma calls
inside the loop (use the batched results and mapping instead).
- Around line 299-360: The code creates an N×M performance problem by querying
courses per module and then calling loaders.courseTags.load and
loaders.courseSponsors.load inside the course loop; instead, collect all
study_module_ids from filteredModules, fetch all courseStudyModule rows once
(prisma.courseStudyModule.findMany) and then fetch all related courses in a
single prisma.course.findMany using those course IDs, build a map of moduleId ->
courseList, and then call loaders.courseTags.loadMany /
loaders.courseSponsors.loadMany (or batch the load keys) for all courses at once
to populate tags and sponsors; finally transform translations and assign
module.courses from the precomputed map so no per-module or per-course DB/loader
calls occur inside nested loops (adjust code around filteredModules,
module.courses, prisma.courseStudyModule.findMany, prisma.course.findMany,
loaders.courseTags.load, loaders.courseSponsors.load).
In @frontend/components/NewLayout/Courses/CourseCard.tsx:
- Around line 18-24: Update the NewFrontpageCourseFields / NewCourseFields type
in lib/api-types.ts so its field types match the GraphQL fragment: change ects
from number | null to string | null, change upcoming_active_link from string |
null to boolean | null, and change status from string | null to the CourseStatus
enum type; ensure the exported NewCourseFields (or NewFrontpageCourseFields)
definition and any imports reference CourseStatus so existing code like
parseInt(course.ects) and comparisons to CourseStatus compile correctly.
In @frontend/lib/with-apollo-client/fetch-user-details.ts:
- Around line 7-9: Remove the unused _pollo parameter from the fetchUserDetails
function signature (change export default async function
fetchUserDetails(_pollo: ApolloClient<object>) to export default async function
fetchUserDetails()) and update its return typing as needed (still
Promise<CurrentUserQuery["currentUser"]>); then update the single call site in
index.tsx to call fetchUserDetails() without passing the Apollo client. Ensure
any related imports or TypeScript references that assumed that parameter are
adjusted accordingly.
🧹 Nitpick comments (5)
frontend/lib/api-client.ts (2)
32-38: Consider parsing the response body for more informative error messages.When the API returns an error, the response body may contain a more descriptive error message from the server. Currently, only
statusTextis used, which is often generic (e.g., "Bad Request").♻️ Suggested improvement
if (!response.ok) { + let message = `API request failed: ${response.statusText}` + try { + const errorBody = await response.json() + if (errorBody.message) { + message = errorBody.message + } + } catch { + // ignore parse errors, use default message + } const error: ApiError = { - message: `API request failed: ${response.statusText}`, + message, status: response.status, } throw error }
44-75: Extract repeated query parameter building logic into a helper.The same URLSearchParams construction pattern is duplicated across
getFrontpage,getCourses, andgetStudyModules. This could be simplified with a small helper.♻️ Suggested refactor
+function buildQueryString(params: Record<string, string | undefined>): string { + const searchParams = new URLSearchParams() + for (const [key, value] of Object.entries(params)) { + if (value) { + searchParams.append(key, value) + } + } + const query = searchParams.toString() + return query ? `?${query}` : "" +} + export const apiClient = { getFrontpage: (language?: string): Promise<FrontpageResponse> => { - const params = new URLSearchParams() - if (language) { - params.append("language", language) - } - const query = params.toString() - return fetchApi<FrontpageResponse>( - `/api/public/frontpage${query ? `?${query}` : ""}`, - ) + return fetchApi<FrontpageResponse>( + `/api/public/frontpage${buildQueryString({ language })}`, + ) }, // Apply same pattern to getCourses and getStudyModules... }frontend/hooks/usePublicData.ts (1)
44-47: Consider exposing loading/error state fromuseCurrentUserData.This derived hook returns
nullboth when the user is not logged in and when the data is still loading or errored. Consumers may need to differentiate these states.♻️ Alternative that exposes more state
-export function useCurrentUserData(): CurrentUserResponse["currentUser"] { - const { data } = useCurrentUser() - return data?.currentUser ?? null +export function useCurrentUserData() { + const { data, isLoading, error } = useCurrentUser() + return { + currentUser: data?.currentUser ?? null, + isLoading, + error, + } }Alternatively, if the simple return type is intentional and consumers use
useCurrentUser()directly when they need loading state, this is acceptable as-is.frontend/components/NewLayout/Courses/CourseGrid.tsx (2)
290-292: Unnecessary intermediate variables - consider usingcoursesDatadirectly.
tagsDataandtagsLoadingare just aliases forcoursesDataandcoursesLoading. Unless this is scaffolding for a future separate tags endpoint, consider using the original variables directly for clarity.
351-361: Remove unused dependency fromuseCallback.
filteredStatusesis listed as a dependency but is not used in the callback body - the callback correctly uses the functional update formsetFilteredStatuses((oldStatuses) => ...). This causes unnecessary callback recreation.♻️ Proposed fix
const handleStatusChange = useCallback( (status: CourseStatus) => () => { setFilteredStatuses((oldStatuses) => { if (oldStatuses.includes(status)) { return oldStatuses.filter((s) => s !== status) } return [...oldStatuses, status] }) }, - [filteredStatuses], + [], )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
backend/api/index.tsbackend/api/routes/index.tsbackend/api/routes/public.tsfrontend/components/NewLayout/Courses/CourseCard.tsxfrontend/components/NewLayout/Courses/CourseGrid.tsxfrontend/components/NewLayout/Frontpage/Modules/ModuleNavigation.tsxfrontend/components/NewLayout/Frontpage/SelectedCourses.tsxfrontend/components/NewLayout/Modules/StudyModuleList.tsxfrontend/hooks/usePublicData.tsfrontend/lib/api-client.tsfrontend/lib/api-types.tsfrontend/lib/with-apollo-client/fetch-user-details.tsfrontend/package.jsonfrontend/pages/_app.tsx
🧰 Additional context used
🧬 Code graph analysis (10)
frontend/lib/with-apollo-client/fetch-user-details.ts (2)
frontend/graphql/generated/types.ts (1)
CurrentUserQuery(5466-5466)frontend/lib/api-client.ts (1)
apiClient(43-80)
backend/api/index.ts (1)
backend/api/routes/public.ts (1)
PublicController(19-392)
frontend/components/NewLayout/Frontpage/SelectedCourses.tsx (1)
frontend/hooks/usePublicData.ts (1)
useCoursesData(19-25)
frontend/lib/api-client.ts (1)
frontend/lib/api-types.ts (4)
FrontpageResponse(118-121)CoursesResponse(123-126)StudyModulesResponse(128-130)CurrentUserResponse(132-146)
frontend/components/NewLayout/Modules/StudyModuleList.tsx (1)
frontend/hooks/usePublicData.ts (1)
useStudyModulesData(27-33)
frontend/components/NewLayout/Courses/CourseCard.tsx (1)
frontend/lib/api-types.ts (1)
NewCourseFields(97-101)
frontend/components/NewLayout/Frontpage/Modules/ModuleNavigation.tsx (1)
frontend/hooks/usePublicData.ts (1)
useFrontpageData(11-17)
backend/api/routes/public.ts (3)
backend/api/types.ts (1)
ApiContext(5-5)backend/loaders/createLoaders.ts (1)
createLoaders(12-25)frontend/util/guards.ts (1)
isDefined(22-24)
frontend/pages/_app.tsx (1)
frontend/contexts/LoginStateContext.tsx (1)
LoginStateProvider(63-102)
frontend/components/NewLayout/Courses/CourseGrid.tsx (4)
frontend/lib/api-types.ts (2)
TagCoreFields(34-41)NewCourseFields(97-101)frontend/hooks/useQueryParameter.tsx (1)
useQueryParameter(17-51)frontend/hooks/usePublicData.ts (1)
useCoursesData(19-25)frontend/components/NewLayout/Courses/common.tsx (1)
allowedLanguages(7-7)
⏰ 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 (18)
backend/api/routes/public.ts (1)
367-391: LGTM! Clean error handling and appropriate field selection.The
currentUserendpoint properly handles authentication errors by returning null, and exposes only the necessary user fields.backend/api/routes/index.ts (1)
6-6: LGTM! Standard barrel export pattern.The addition of the public routes export follows the existing pattern and correctly exposes the new PublicController.
backend/api/index.ts (1)
8-8: LGTM! Clean integration of PublicController.The new public routes are properly wired with:
- Correct import and instantiation
- RESTful endpoint naming (
/public/frontpage,/public/courses, etc.)- Consistent pattern with existing controllers
Also applies to: 19-19, 22-25
frontend/pages/_app.tsx (2)
49-59: LGTM! Correct React Query setup pattern.The QueryClient is properly initialized using
useStateto ensure stability across renders, following React Query best practices. The configuration is reasonable:
- 5-minute staleTime reduces unnecessary refetches
- Disabled
refetchOnWindowFocusis appropriate for this application's UX
106-115: LGTM! Provider correctly wraps the application.The QueryClientProvider is properly positioned in the component tree, wrapping all components that will use React Query hooks while preserving the existing provider hierarchy.
frontend/package.json (1)
42-42: Version verified as valid with no known vulnerabilities.The package
@tanstack/[email protected]exists on the npm registry and has no known security vulnerabilities. The version is not deprecated and carries an MIT license.frontend/components/NewLayout/Modules/StudyModuleList.tsx (2)
11-11: LGTM: Clean migration to custom hook.The import of
useStudyModulesDataaligns with the broader migration from Apollo GraphQL to React Query-based data fetching.
49-49: LGTM: Proper hook integration with backward-compatible naming.The destructuring pattern
isLoading: loadingmaintains compatibility with the existing component logic while adopting the new data-fetching approach.frontend/lib/with-apollo-client/fetch-user-details.ts (1)
10-15: Verify that silent error handling is intentional.The try/catch block returns
nullfor any error without logging or propagating the error. This could mask authentication failures, network issues, or API errors. Ensure that:
- Callers properly handle the
nullcase- Silent failure is the intended behavior
- Consider logging errors for debugging purposes
frontend/components/NewLayout/Frontpage/SelectedCourses.tsx (2)
20-20: LGTM: Consistent migration pattern.The import of
useCoursesDatafollows the same pattern as other components in this PR.
109-109: LGTM: Consistent hook usage.The migration to
useCoursesDatafollows the established pattern with proper destructuring.frontend/components/NewLayout/Frontpage/Modules/ModuleNavigation.tsx (2)
7-7: LGTM: Clean import.The migration to
useFrontpageDatais consistent with the overall PR approach.
80-80: LGTM: Proper hook integration.The hook usage follows the same pattern as other migrated components with appropriate variable naming.
frontend/lib/api-client.ts (1)
14-41: LGTM on the core fetch logic.The
fetchApihelper properly attaches the Bearer token when available, allows header overrides, and normalizes errors into a consistentApiErrorstructure.frontend/hooks/usePublicData.ts (1)
11-42: LGTM - well-structured React Query hooks.The hooks properly include
languagein query keys for correct cache differentiation, and the stale times are appropriate (5 min for public data, 1 min for user data). Disabling retry foruseCurrentUseris a good choice since auth failures shouldn't be retried automatically.frontend/lib/api-types.ts (1)
1-146: LGTM - comprehensive and well-structured type definitions.The interfaces properly model the API response shapes with:
- Appropriate use of nullable types (
| null) for optional fields- Clean inheritance (
NewCourseFields extends NewFrontpageCourseFields,StudyModuleFieldsWithCourses extends StudyModuleFields)- Consistent naming conventions following
snake_caseto match backend API responsesfrontend/components/NewLayout/Courses/CourseGrid.tsx (2)
276-281: Good use of type guard for runtime validation.The
typedInitialStatusesfilter properly validates that query parameter values matchCourseStatusenum values before use, preventing potential type mismatches at runtime.
287-289: Clean migration to React Query-based data fetching.The integration with
useCoursesDatais well-implemented, with proper loading state handling and memoized tag aggregation logic.Also applies to: 323-349
| React.useEffect(() => { | ||
| if (tagsData?.tags) { | ||
| if (initialActiveTags) { | ||
| setActiveTags( | ||
| tagsData.tags.filter((tag) => initialActiveTags.includes(tag.id)) ?? | ||
| [], | ||
| ) | ||
| } else { | ||
| const defaultLanguageTag = tagsData.tags.find( | ||
| (tag) => tag.id === locale && tag.types?.includes("language"), | ||
| ) | ||
| if (defaultLanguageTag) { | ||
| setActiveTags([defaultLanguageTag]) | ||
| } | ||
| }, | ||
| }, | ||
| ) | ||
| } | ||
| } | ||
| }, [tagsData, initialActiveTags, locale]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for useQueryParameter hook definition
rg -l "useQueryParameter" --type tsx --type ts | head -20Repository: rage/mooc.fi
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Search for useQueryParameter function definition
rg "export.*useQueryParameter|function useQueryParameter|const useQueryParameter" -A 20 --type tsx --type tsRepository: rage/mooc.fi
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find files that mention useQueryParameter
rg "useQueryParameter" --type-list | grep -i tsRepository: rage/mooc.fi
Length of output: 445
🏁 Script executed:
#!/bin/bash
# Search for useQueryParameter without file type restrictions
rg "export.*useQueryParameter|function useQueryParameter|const useQueryParameter" -A 20Repository: rage/mooc.fi
Length of output: 2026
🏁 Script executed:
#!/bin/bash
# Find TypeScript files in frontend
fd "\.(ts|tsx|js|jsx)$" frontend | head -30Repository: rage/mooc.fi
Length of output: 1364
🏁 Script executed:
#!/bin/bash
# Read the complete useQueryParameter hook
cat -n frontend/hooks/useQueryParameter.tsxRepository: rage/mooc.fi
Length of output: 1700
🏁 Script executed:
#!/bin/bash
# Find where initialActiveTags is used/declared in CourseGrid.tsx
rg "initialActiveTags" frontend/components/NewLayout/Courses/CourseGrid.tsx -B 5 -A 2Repository: rage/mooc.fi
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Check the full context of CourseGrid.tsx around initialActiveTags declaration
rg "initialActiveTags|useMemo|useCallback" frontend/components/NewLayout/Courses/CourseGrid.tsx -B 2 -A 2Repository: rage/mooc.fi
Length of output: 1571
Consider memoizing initialActiveTags to prevent unnecessary effect re-executions.
useQueryParameter with array: true returns a new array reference on every render (from .map() or [] literals). Since initialActiveTags is a dependency in the useEffect, the effect runs unnecessarily on every render even when the query parameter values haven't changed.
Wrap the result with useMemo:
Example fix
const initialActiveTags = useMemo(
() => useQueryParameter("tag", { enforce: false, array: true }),
[]
)| export default async function fetchUserDetails( | ||
| apollo: ApolloClient<object>, | ||
| _pollo: ApolloClient<object>, | ||
| ): Promise<CurrentUserQuery["currentUser"]> { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "fetchUserDetails\(" --type ts --type tsx -A 2 -B 2Repository: rage/mooc.fi
Length of output: 83
🏁 Script executed:
rg "fetchUserDetails\(" -A 2 -B 2 --type tsRepository: rage/mooc.fi
Length of output: 918
🏁 Script executed:
# Check the complete function implementation
cat -n frontend/lib/with-apollo-client/fetch-user-details.ts
# Search for all references to fetchUserDetails (not just direct calls)
rg "fetchUserDetails" --type tsRepository: rage/mooc.fi
Length of output: 869
Remove the unused _pollo parameter from the function signature and update the call site.
The Apollo client parameter is never used in the function body—it only calls apiClient.getCurrentUser(). Only one call site exists in index.tsx, so update it to not pass the Apollo parameter.
🤖 Prompt for AI Agents
In @frontend/lib/with-apollo-client/fetch-user-details.ts around lines 7 - 9,
Remove the unused _pollo parameter from the fetchUserDetails function signature
(change export default async function fetchUserDetails(_pollo:
ApolloClient<object>) to export default async function fetchUserDetails()) and
update its return typing as needed (still
Promise<CurrentUserQuery["currentUser"]>); then update the single call site in
index.tsx to call fetchUserDetails() without passing the Apollo client. Ensure
any related imports or TypeScript references that assumed that parameter are
adjusted accordingly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1302 +/- ##
==========================================
+ Coverage 62.59% 62.83% +0.23%
==========================================
Files 153 154 +1
Lines 6422 6592 +170
Branches 1579 1639 +60
==========================================
+ Hits 4020 4142 +122
- Misses 2244 2295 +51
+ Partials 158 155 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @backend/api/routes/public.ts:
- Line 249: The Prisma orderBy entries incorrectly treat the Int field `order`
as a nested object (e.g. `orderBy: [{ order: { sort: "asc" } }]`); update each
occurrence to use the scalar syntax `orderBy: [{ order: "asc" }]` (replace the
`{ order: { sort: "asc" } }` instances with `{ order: "asc" }`) in the relevant
query handlers in this file so ordering uses the Int `order` field correctly.
In @frontend/lib/api-types.ts:
- Around line 120-123: FrontpageResponse currently types courses as
Array<NewCourseFields>, but NewCourseFields requires course_translations while
the backend frontpage endpoint calls enrichCourses({ includeStudyModules: true
}) without includeTranslations; fix by making the frontend type match backend:
update FrontpageResponse to use a course type where course_translations is
optional (or a slimmer type), or alternatively update the backend enrichCourses
call in the frontpage endpoint (enrichCourses) to pass includeTranslations: true
so NewCourseFields is fully populated; locate and change either the
FrontpageResponse declaration or the enrichCourses call to keep the shapes
consistent (symbols: FrontpageResponse, NewCourseFields, enrichCourses,
includeStudyModules, includeTranslations, frontpage).
In @frontend/lib/with-apollo-client/index.tsx:
- Line 85: The early-return condition using the nullish coalescing operator
changed behavior; replace "ctx?.res?.headersSent ?? ctx?.res?.finished" with a
logical-or check to preserve the original semantics so the code returns early if
either headersSent or finished is truthy; specifically update the conditional
where ctx?.res?.headersSent and ctx?.res?.finished are evaluated (in the
withApollo client initialization/handler) to use "||" or an explicit boolean
expression like "(ctx?.res?.headersSent || ctx?.res?.finished)" to ensure the
same early-return behavior as before.
🧹 Nitpick comments (1)
frontend/lib/with-apollo-client/fetch-user-details.ts (1)
5-13: Clean migration from Apollo to API client; consider aligning return type.The function correctly uses
apiClient.getCurrentUser()and handles errors gracefully by returningnull. The previous review concern about the unused parameter has been addressed.However, the return type uses
CurrentUserQuery["currentUser"](from GraphQL types) while the actual data comes fromCurrentUserResponse(from API types). Consider usingCurrentUserResponse["currentUser"]for type consistency, unless existing call sites rely on the GraphQL-generated type.♻️ Optional: Align return type with API types
-import { CurrentUserQuery } from "/graphql/generated" +import { CurrentUserResponse } from "/lib/api-types" export default async function fetchUserDetails(): Promise< - CurrentUserQuery["currentUser"] + CurrentUserResponse["currentUser"] > {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/api/routes/public.tsfrontend/lib/api-types.tsfrontend/lib/with-apollo-client/fetch-user-details.tsfrontend/lib/with-apollo-client/index.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
frontend/lib/with-apollo-client/index.tsx (1)
frontend/lib/with-apollo-client/fetch-user-details.ts (1)
fetchUserDetails(5-14)
frontend/lib/with-apollo-client/fetch-user-details.ts (2)
frontend/graphql/generated/types.ts (1)
CurrentUserQuery(5466-5466)frontend/lib/api-client.ts (1)
apiClient(43-80)
backend/api/routes/public.ts (4)
backend/loaders/createLoaders.ts (1)
createLoaders(12-25)backend/loaders/courseSponsorsLoader.ts (1)
LoadedCourseSponsor(12-12)frontend/util/guards.ts (1)
isDefined(22-24)backend/api/types.ts (1)
ApiContext(5-5)
⏰ 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 (7)
backend/api/routes/public.ts (4)
43-68: Well-structured helper function addressing previous duplication concerns.The
filterCoursesWithTranslationsfunction properly extracts the repeated course filtering logic. The translation fallback chain (requested language → English default → first available) is a good pattern.
70-161: Excellent refactoring that addresses previous N+1 query concerns.The
enrichCoursesfunction now:
- Uses
loadManyfor batch loading tags and sponsors (lines 88-91)- Fetches study modules via Prisma's relation includes in a single query (lines 100-112)
- Gracefully handles DataLoader errors with fallback to empty arrays
This eliminates the per-course database queries previously flagged.
440-506: Batch-fetching pattern properly addresses previous N×M query concern.The study modules endpoint now:
- Fetches all module-course relationships in one query (lines 443-454)
- Collects unique course IDs and fetches all courses once (lines 464-481)
- Enriches all courses in a single batch (lines 488-497)
- Maps enriched courses back to modules (lines 501-506)
This eliminates the nested loop database queries previously flagged.
513-543: Verify intentional exposure of sensitive fields in public response.The
currentUserendpoint exposesreal_student_number(line 538) in the response. While this is the authenticated user's own data, ensure this field should be returned via a public API endpoint rather than a more restricted authenticated endpoint.frontend/lib/api-types.ts (2)
134-147: Type mismatch: Backend returnsDatebut frontend expectsstringfor timestamps.The backend
CurrentUserResponse(backend file lines 228-229) definescreated_atandupdated_atasDate, but this frontend interface expectsstring. JSON serialization will convertDateto ISO string, so the frontend type is correct for the actual wire format. However, verify that any code consuming these values handles them as strings rather thanDateobjects.
1-68: Well-structured type definitions.The core field interfaces provide a clean and consistent API contract. The proper use of nullable types (
| null) and the separation of concerns (e.g.,SponsorTranslationFields,SponsorImageFieldscomposed intoCourseSponsorFields) is good practice.frontend/lib/with-apollo-client/index.tsx (1)
71-71: Call signature updated to match newfetchUserDetailsimplementation.The call now correctly omits the
apolloparameter sincefetchUserDetailsinternally usesapiClient.getCurrentUser()instead of the Apollo client.
|
|
||
| if (typeof window === "undefined") { | ||
| if (ctx?.res?.headersSent || ctx?.res?.finished) { | ||
| if (ctx?.res?.headersSent ?? ctx?.res?.finished) { |
There was a problem hiding this comment.
Behavioral change: ?? vs || alters early-return logic.
The change from || to ?? has different semantics:
- Old:
headersSent || finished— returns early if either is truthy - New:
headersSent ?? finished— usesfinishedonly ifheadersSentisnull/undefined
If headersSent is false but finished is true, the old code would return early, but the new code will not return early and continue rendering. Verify this is the intended behavior.
🔧 Suggested fix if original behavior was intended
- if (ctx?.res?.headersSent ?? ctx?.res?.finished) {
+ if (ctx?.res?.headersSent || ctx?.res?.finished) {📝 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.
| if (ctx?.res?.headersSent ?? ctx?.res?.finished) { | |
| if (ctx?.res?.headersSent || ctx?.res?.finished) { |
🤖 Prompt for AI Agents
In @frontend/lib/with-apollo-client/index.tsx at line 85, The early-return
condition using the nullish coalescing operator changed behavior; replace
"ctx?.res?.headersSent ?? ctx?.res?.finished" with a logical-or check to
preserve the original semantics so the code returns early if either headersSent
or finished is truthy; specifically update the conditional where
ctx?.res?.headersSent and ctx?.res?.finished are evaluated (in the withApollo
client initialization/handler) to use "||" or an explicit boolean expression
like "(ctx?.res?.headersSent || ctx?.res?.finished)" to ensure the same
early-return behavior as before.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/api/routes/public.ts:
- Line 250: The orderBy entries use an incorrect nested object for the scalar
field `order` (e.g., `orderBy: [{ order: { sort: "asc" } }]`); change each to
use the scalar sort syntax `orderBy: [{ order: "asc" }]` for the occurrence
shown and the two other instances referenced (the similar usages at the other
locations should be updated the same way).
🧹 Nitpick comments (1)
frontend/lib/api-client.ts (1)
14-43: Consider adding error handling for JSON parsing.The
response.json()call at line 42 can throw if the response body isn't valid JSON, but this isn't caught. If the backend returns a non-JSON error response, this could cause an unhandled promise rejection.🛡️ Proposed enhancement
if (!response.ok) { const error: ApiError = { message: `API request failed: ${response.statusText}`, status: response.status, } throw error } - return response.json() + try { + return response.json() + } catch (e) { + const error: ApiError = { + message: `Failed to parse API response as JSON`, + status: response.status, + } + throw error + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/api/routes/public.tsfrontend/lib/api-client.tsfrontend/lib/api-types.tsfrontend/lib/with-apollo-client/fetch-user-details.tsfrontend/lib/with-apollo-client/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/api-types.ts
🧰 Additional context used
🧬 Code graph analysis (4)
frontend/lib/with-apollo-client/fetch-user-details.ts (2)
frontend/lib/api-types.ts (1)
CurrentUserResponse(134-149)frontend/lib/api-client.ts (1)
apiClient(45-86)
frontend/lib/with-apollo-client/index.tsx (1)
frontend/lib/with-apollo-client/fetch-user-details.ts (1)
fetchUserDetails(4-13)
backend/api/routes/public.ts (6)
frontend/graphql/generated/types.ts (2)
CourseTranslation(889-901)Image(1714-1728)backend/loaders/createLoaders.ts (1)
createLoaders(12-25)backend/loaders/courseSponsorsLoader.ts (1)
LoadedCourseSponsor(12-12)frontend/util/guards.ts (1)
isDefined(22-24)frontend/lib/api-types.ts (3)
FrontpageResponse(120-123)CoursesResponse(125-128)StudyModulesResponse(130-132)backend/api/types.ts (1)
ApiContext(5-5)
frontend/lib/api-client.ts (1)
frontend/lib/api-types.ts (4)
FrontpageResponse(120-123)CoursesResponse(125-128)StudyModulesResponse(130-132)CurrentUserResponse(134-149)
⏰ 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 (12)
frontend/lib/with-apollo-client/index.tsx (1)
71-71: LGTM! Function call updated correctly.The change from
fetchUserDetails(apollo)tofetchUserDetails(accessToken)correctly aligns with the refactored function signature infetch-user-details.tsthat now uses the API client instead of Apollo.frontend/lib/with-apollo-client/fetch-user-details.ts (1)
1-13: LGTM! Clean refactor from Apollo to API client.The function has been correctly refactored to:
- Accept
accessTokeninstead of Apollo client- Use the centralized
apiClient.getCurrentUser()- Include proper error handling that returns
nullon failure- Align with the new type system (
CurrentUserResponse)The implementation is clean and consistent with the broader migration to REST endpoints.
frontend/lib/api-client.ts (3)
9-12: LGTM! Clean error interface.The
ApiErrorinterface provides a simple and effective structure for API errors with message and optional status code.
46-66: LGTM! Consistent query parameter handling.The
getFrontpageandgetCoursesmethods follow a clean, consistent pattern for handling optional language query parameters with proper type safety.
68-86: LGTM! Clean endpoint implementations.The
getStudyModulesandgetCurrentUsermethods are well-implemented:
getStudyModulesfollows the same pattern as other endpointsgetCurrentUsercorrectly passes theaccessTokenparameter through tofetchApibackend/api/routes/public.ts (7)
43-68: LGTM! Robust translation fallback logic.The
filterCoursesWithTranslationshelper correctly handles translation selection with a sensible fallback chain: requested language → DEFAULT_LANGUAGE → first available translation. The defensive programming and use ofisDefinedensures type safety.
70-161: LGTM! Effective batching eliminates N+1 queries.The
enrichCourseshelper properly addresses the N+1 query concerns mentioned in past reviews:
- Uses
loadManyto batch-fetch tags and sponsors for all courses at once (lines 88-91)- Uses Prisma's fluent API with
selectto batch-fetch study modules (lines 100-117)- Includes graceful error handling for DataLoader failures (lines 125-131)
This implementation avoids the per-course query pattern and should perform well even with many courses.
169-188: LGTM! Simple and consistent translation handling.The
filterStudyModulesWithTranslationshelper follows the same defensive pattern as the course filtering, handling missing translations gracefully.
260-322: Endpoint logic is well-structured.The
frontpageendpoint has clean logic flow:
- Fetches visible courses and study modules
- Applies language filtering when specified
- Uses helper functions for translation and enrichment
- Returns properly shaped response
The batching improvements eliminate the N+1 query issues mentioned in past reviews.
However, note the
orderBysyntax issue at line 276 (same as line 250).
324-395: LGTM! Consistent endpoint structure with proper tag handling.The
coursesendpoint follows the same clean pattern asfrontpage, with appropriate additions:
- Fetches and filters tags with translation support (lines 339-351)
- Includes course translations in the response via
includeTranslations: true(line 362)- Uses consistent translation fallback logic for tags (lines 366-389)
397-512: LGTM! Excellent batching strategy eliminates nested query problem.The
studyModulesendpoint effectively addresses the N×M performance issue mentioned in past reviews:
- Collects all module-to-course relationships upfront (lines 444-463)
- Batch-fetches all unique courses in a single query (lines 471-482)
- Enriches all courses together (lines 489-498)
- Uses Map-based lookup for O(1) assignment (lines 500-507)
This approach completely avoids the nested loop + per-iteration query pattern.
However, note the
orderBysyntax issue at line 411 (same as lines 250 and 276).
514-545: LGTM! Clean user endpoint with appropriate authentication handling.The
currentUserendpoint is well-implemented:
- Uses
getUserfrom base Controller for authentication (line 518)- Returns
nullfor unauthenticated users, which is the expected behavior (lines 520-522)- Properly computes
full_namefrom name components (lines 532-535)- Response shape matches the
CurrentUserResponseinterface
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @backend/api/routes/__test__/public.test.ts:
- Around line 289-301: The test in the "returns study modules ordered by
order.sort" case incorrectly treats the module "order" as an object (modules[i -
1].order?.sort) but "order" is a numeric scalar; update the comparisons to use
the scalar value (e.g., prevOrder = modules[i - 1].order ?? Infinity and
currOrder = modules[i].order ?? Infinity) when asserting non-decreasing order
for response.data.study_modules returned by getStudyModules().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/api/routes/__test__/public.test.tsbackend/api/routes/public.tsfrontend/lib/api-types.ts
⏰ 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). (3)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript)
- GitHub Check: build-deploy
🔇 Additional comments (18)
backend/api/routes/public.ts (9)
1-42: LGTM! Clean type definitions and imports.The type definitions clearly document the data transformations through the pipeline (CourseWithTranslations → FilteredCourse → EnrichedCourse), and the DEFAULT_LANGUAGE constant provides a clear fallback strategy.
43-68: LGTM! Well-structured translation helper.The translation fallback chain is comprehensive and defensive, handling edge cases where translations might be missing. The extraction of this logic into a reusable helper addresses the code duplication concerns from previous reviews.
70-161: LGTM! Efficient batched enrichment logic.The use of
loadManyfor tags and sponsors, combined with Prisma's relation select for study modules, effectively addresses the N+1 query concerns from previous reviews. The error handling and conditional enrichment based on options make this a robust, reusable helper.
169-188: LGTM! Consistent translation extraction.The helper properly extracts and flattens study module translations, maintaining consistency with the course translation approach.
190-258: LGTM! Well-organized controller structure.The extraction of
getVisibleCoursesas a private helper method eliminates duplication between the frontpage and courses endpoints. The response type interfaces provide clear API contracts.
260-322: LGTM! Clean frontpage endpoint implementation.The endpoint properly leverages the extracted helpers and batched enrichment logic. The conditional language filtering for study modules is well-structured.
324-395: LGTM! Comprehensive courses endpoint.The endpoint appropriately includes both study modules and translations in the enriched courses. The tag translation fallback logic is consistent with the course translation approach.
397-512: LGTM! Efficient batched implementation.The studyModules endpoint effectively addresses the N×M query concern from previous reviews by batch-fetching all courses upfront and mapping them back to their respective modules. The approach eliminates nested per-module and per-course database calls.
514-545: LGTM! Robust user data handling.The currentUser endpoint properly handles unauthenticated requests by returning null, and the full_name computation correctly handles all combinations of first_name and last_name presence.
backend/api/routes/__test__/public.test.ts (5)
1-39: LGTM! Well-structured test setup.The test helpers cleanly abstract endpoint access with optional language parameters, and the beforeEach hook properly seeds test data for each test case.
40-125: LGTM! Comprehensive frontpage test coverage.The tests thoroughly validate the frontpage endpoint behavior including authentication, field structure, visibility filtering, language handling, and ordering. The ordering test correctly treats
orderas a scalar field.
127-212: LGTM! Thorough courses endpoint validation.The tests appropriately verify the courses endpoint's additional data (course_translations, tags with tag_translations) beyond what the frontpage endpoint returns.
214-288: LGTM! Solid study modules test coverage.The tests validate study module structure, nested courses with enriched data, visibility filtering, and language handling.
304-381: LGTM! Complete current-user endpoint validation.The tests thoroughly validate authentication states (unauthenticated, normal user, admin) and comprehensively test the full_name computation logic, including all edge cases for missing first_name or last_name.
frontend/lib/api-types.ts (4)
1-69: LGTM! Comprehensive core field definitions.The interfaces thoroughly model all atomic data shapes with appropriate nullability, providing a solid foundation for the higher-level response types.
70-109: LGTM! Well-structured course type hierarchy.The three-tiered course type structure properly models the different levels of data returned by each endpoint:
NewFrontpageCourseFields: base fields with study modulesNewCourseFields: full data with required translationsFrontpageCourseFields: tags/sponsors with optional translationsThe use of
FrontpageCourseFields(with optionalcourse_translations) for the frontpage response correctly aligns with the backend's behavior, addressing the type mismatch concern noted in previous reviews.
111-138: LGTM! Response types correctly model each endpoint.The response interfaces appropriately use different course shapes based on each endpoint's data enrichment level, ensuring type safety matches the actual backend responses.
140-155: LGTM! CurrentUserResponse accurately models the endpoint.The interface properly handles both authenticated (user object) and unauthenticated (null) states, with all fields correctly typed to match the backend response.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.