Feat/552 rs app courses integration #633
Conversation
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
Lighthouse Report:
|
Quiddlee
requested review from
andron13,
dzmitry-varabei,
ansivgit and
natanchik
as code owners
|
Warning Rate limit exceeded@Quiddlee has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 39 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. 📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes to enhance the handling of course data within the application. Key modifications include the addition of an Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (32)
env.d.ts (1)
2-4: Consider adding JSDoc documentation.Adding JSDoc comments would help developers understand the expected format and requirements of the API_URL.
declare namespace NodeJS { interface ProcessEnv { + /** + * URL for the RS APP API integration + * @example "https://api.example.com/v1" + */ API_URL: string; } }src/entities/course/constants.ts (1)
1-1: Consider strengthening the regular expression pattern.The current pattern could be enhanced to prevent partial matches and validate quarter numbers.
-export const ALIAS_QUARTER_REGEXP = new RegExp(/-\d{4}q\d/, 'i'); +export const ALIAS_QUARTER_REGEXP = new RegExp(/^-\d{4}q[1-4]$/, 'i');src/app/courses/page.tsx (2)
11-11: Consider implementing error boundaries.Since the Courses component now likely handles its own data fetching, consider implementing error boundaries to gracefully handle potential API failures.
Line range hint
4-8: Consider dynamic metadata generation.The metadata is currently static. Since we're moving to dynamic course data, consider enhancing SEO by generating dynamic metadata based on the fetched courses.
src/shared/helpers/getCourseDate.ts (1)
9-9: Consider adding type safety for startDate parameter.The function logic is correct, but consider adding runtime validation for the startDate parameter to ensure it's not undefined or null.
export const getCourseDate = (startDate: string, staleAfterDays: number) => { + if (!startDate) return TO_BE_DETERMINED; const staleDate = dayjs().subtract(staleAfterDays, 'day');src/core/base-layout/base-layout.tsx (2)
10-10: Consider memoizing courses propSince courses are fetched at the layout level and passed down, consider memoizing this data to prevent unnecessary re-renders of child components.
Performance impact confirmed - action required
The
getCoursesfetch is indeed used in multiple components that could affect page load time:
base-layout.tsx(affects all pages)upcoming-courses.tsxcourses.tsxfooter.tsxanddesktop-view.tsxNo React Suspense boundaries or loading states were found in the codebase, making this a real performance concern.
🔗 Analysis chain
Line range hint
5-14: Verify impact on page loading performanceThe layout now blocks rendering until courses are fetched. This could impact the initial page load time.
Consider implementing:
- Loading states
- React Suspense boundaries
- Static generation of course data where possible
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if getCourses is used elsewhere in the codebase rg "getCourses" --type ts --type tsx # Look for any existing loading states or suspense boundaries rg "Suspense|useLoading|loading" --type ts --type tsxLength of output: 148
Script:
#!/bin/bash # Check if getCourses is used elsewhere in the codebase without file type restrictions rg "getCourses" # Look for any existing loading states or suspense boundaries rg "Suspense|useLoading|loading" # Find all TypeScript/React files to understand the codebase structure fd "\.(ts|tsx)$"Length of output: 17898
.gitignore (1)
37-37: Consider adding more environment file patterns.For completeness, consider adding other common environment file variants.
# env .env +.env.local +.env.development.local +.env.test.local +.env.production.localsrc/app/courses/nodejs/page.tsx (1)
13-17: Consider adding loading stateThe async implementation is correct, but since this is a page component, consider adding a loading.tsx file in the same directory to handle the loading state during data fetch.
// src/app/courses/nodejs/loading.tsx export default function Loading() { return <div>Loading course information...</div>; }src/app/courses/angular/page.tsx (1)
13-17: Consider adding error handling for course fetching.While the async implementation is correct, adding error handling would improve reliability.
export default async function AngularRoute() { - const course = await selectCourse(courseName); + try { + const course = await selectCourse(courseName); + return <Angular course={course} courseName={courseName} />; + } catch (error) { + console.error('Failed to fetch Angular course:', error); + throw error; // Let error boundary handle it + } - return <Angular course={course} courseName={courseName} />; }src/app/courses/aws-devops/page.tsx (1)
13-17: Consider adding error handling for async operations.While the async implementation looks good, consider adding error boundaries or try-catch blocks to handle potential API failures gracefully.
export default async function AwsDeveloperRoute() { - const course = await selectCourse(courseName); + try { + const course = await selectCourse(courseName); + return <AwsDevOps course={course} courseName={courseName} />; + } catch (error) { + console.error('Failed to load course:', error); + // Consider adding an error component or fallback UI + throw error; + } - return <AwsDevOps course={course} courseName={courseName} />; }src/app/courses/javascript-ru/page.tsx (1)
13-17: Consider adding error handlingWhile the async implementation is correct, consider adding error handling for cases where course data fetch fails.
export default async function JsRuRoute() { - const course = await selectCourse(courseName); + try { + const course = await selectCourse(courseName); + return <JavaScriptRu lang="ru" course={course} courseName={courseName} />; + } catch (error) { + console.error('Failed to load course:', error); + throw error; // Let error boundary handle it + } - return <JavaScriptRu lang="ru" course={course} courseName={courseName} />; }src/entities/course/api/course-api.ts (3)
4-4: Consider adding cache invalidation mechanism.The module-level cache persists indefinitely. For production environments with SSG this is fine, but during development it might lead to stale data.
-let cache: Course[] | null = null; +let cache: { data: Course[] | null; timestamp: number } = { data: null, timestamp: 0 };
12-13: Document the caching strategy.The
force-cachestrategy and JSON parsing could benefit from error handling and documentation.+ // Use force-cache to maximize cache hits in production const data = await fetch(process.env.API_URL, { cache: 'force-cache' }); + // Parse response as ApiCoursesResponse array const courses = (await data.json()) as ApiCoursesResponse[];
18-20: Enhance error handling with specific cases.The current error handling could be more specific about what went wrong.
} catch (e) { - throw new Error(`Something went wrong fetching courses! (${e})`); + if (e instanceof TypeError) { + throw new Error(`Network error or invalid API_URL: ${e.message}`); + } + throw new Error(`Failed to fetch courses: ${e instanceof Error ? e.message : String(e)}`); }src/app/courses/javascript-preschool-ru/page.tsx (1)
Line range hint
13-19: Consider adding error boundary for async operations.While the async course selection is good, ensure there's proper error handling for failed course fetches.
export default async function JsPreRoute() { + try { const course = await selectCourse(courseName); return ( <JavaScriptPreSchoolRu lang="ru" type="pre-school-ru" course={course} courseName={courseName} /> ); + } catch (error) { + // TODO: Add error boundary component + return <ErrorComponent error={error} />; + } }src/entities/course/helpers/sync-with-api-data.ts (2)
1-4: Consider using absolute import path for 'data'While most imports use absolute paths with '@/' prefix, the 'data' import uses a relative path. Consider maintaining consistency by using an absolute path.
18-20: Consider performance optimization
structuredClonemight be expensive for large objects. Since we're only updating thestartDate, consider creating a new object with spread operator instead.- const clonedCourse = structuredClone(course); - clonedCourse.startDate = dayJS(currApiCourse.startDate).format(COURSE_DATE_FORMAT); + const clonedCourse = { + ...course, + startDate: dayJS(currApiCourse.startDate).format(COURSE_DATE_FORMAT) + };src/widgets/courses/ui/courses.test.tsx (1)
9-24: Consider adding more test coverage.While the happy path is well tested, consider adding tests for:
- Loading state verification
- Error handling scenarios
- Snapshot test for UI consistency
Example test cases to add:
it('shows loading state while fetching courses', async () => { const courses = await Courses(); render(courses); expect(screen.getByTestId('loading-spinner')).toBeVisible(); }); it('handles error state gracefully', async () => { // Mock API failure const courses = await Courses(); render(courses); expect(screen.getByTestId('error-message')).toBeVisible(); }); it('matches snapshot', async () => { const courses = await Courses(); const { container } = render(courses); expect(container).toMatchSnapshot(); });src/widgets/upcoming-courses/ui/upcoming-courses.test.tsx (2)
9-12: Consider using waitFor to handle async updates.While awaiting the component is good, you might want to use
waitForto ensure all async updates are complete before assertions.beforeEach(async () => { const upcomingCourses = await UpcomingCourses(); - render(upcomingCourses); + await screen.waitFor(() => + expect(screen.getByTestId('courses-list')).toBeInTheDocument() + ); });
Line range hint
14-42: Add test coverage for edge cases.The current test suite covers happy paths well. Consider adding tests for:
- Error handling
- Loading states
- Empty course list scenario
Example test case:
it('shows loading state initially', () => { render(<UpcomingCourses />); expect(screen.getByTestId('loading-spinner')).toBeInTheDocument(); }); it('handles API errors gracefully', async () => { vi.mocked(getActualData).mockRejectedValueOnce(new Error('API Error')); render(<UpcomingCourses />); expect(await screen.findByText('Failed to load courses')).toBeInTheDocument(); });src/entities/course/types.ts (1)
5-7: Consider adding JSDoc commentsThe type definitions are well-structured, but adding JSDoc comments would improve documentation.
+/** Type representing the structure of course aliases */ type CourseAliases = typeof COURSE_ALIASES; +/** Type representing the possible values of course aliases */ export type CourseAliasValues = CourseAliases[keyof CourseAliases];src/core/base-layout/components/footer/footer.test.tsx (2)
17-22: Add error handling and loading state testsWhile the async rendering is handled, consider adding:
- Error handling test case
- Loading state verification
it('should handle errors in desktop view', async () => { // Mock error case and verify error handling }); it('should show loading state while rendering', async () => { // Verify loading state before content appears });
26-29: Consider adding prop validation testsThe basic rendering test looks good, but consider adding test cases for:
- Invalid course data
- Different type prop values
it('should handle invalid course data gracefully', () => { const { getByTestId } = renderWithRouter(<MobileView courses={[]} type="footer" />); // Verify empty state handling });.github/workflows/production.yml (1)
Line range hint
8-8: Update Node.js version to a stable release.Node.js 22.x is not yet released. Consider using a stable LTS version like 20.x.
Apply this diff:
- NODE_VERSION: '22.x' # set this to the node version to use + NODE_VERSION: '20.x' # set this to the node version to use🧰 Tools
🪛 yamllint
[error] 29-29: trailing spaces
(trailing-spaces)
src/shared/helpers/getActualData.ts (1)
51-52: Consider making the sorting logic more explicit.While the logic is correct, the ternary operation could be clearer.
Consider this more explicit version:
- if (dateA === TO_BE_DETERMINED || dateB === TO_BE_DETERMINED) { - return dateA === TO_BE_DETERMINED ? 1 : -1; + if (dateA === TO_BE_DETERMINED && dateB === TO_BE_DETERMINED) return 0; + if (dateA === TO_BE_DETERMINED) return 1; // Move TBD dates to the end + if (dateB === TO_BE_DETERMINED) return -1;src/widgets/school-menu/ui/school-menu.tsx (2)
4-4: Consider dynamically loading mentorship coursesThe PR aims to move away from static data, but mentorship courses are still imported statically.
Consider fetching mentorship courses dynamically to align with the course data synchronization objective.
46-46: Add JSDoc for the courses propThe new courses prop would benefit from documentation explaining its purpose and requirements.
+ /** Array of course data fetched from RS APP */ courses: Course[];src/widgets/school-menu/school-menu.test.tsx (1)
68-70: Consider extracting magic numbers into constantsThe number 6 for description length and the hardcoded indices could be made more maintainable.
+const EXPECTED_DESCRIPTION_COUNT = 6; -expect(descriptions).toHaveLength(6); +expect(descriptions).toHaveLength(EXPECTED_DESCRIPTION_COUNT);src/core/base-layout/components/header/header.tsx (1)
26-26: Consider using an enum or constants for menu headingsThe heading strings are currently hardcoded. Consider moving them to a constants file for better maintainability and reuse.
+ const MENU_HEADINGS = { + RS_SCHOOL: 'rs school', + ALL_COURSES: 'all courses', + COMMUNITY: 'community', + MENTORSHIP: 'mentorship', + } as const; const navLinks = [ { label: 'RS School', href: ROUTES.HOME, - heading: 'rs school', + heading: MENU_HEADINGS.RS_SCHOOL, }, // ... apply similar changes to other items ];Also applies to: 31-31, 36-36, 41-41
src/shared/helpers/getActualData.test.ts (1)
21-22: Consider diversifying test dataWhile using constants is good, all three mock courses currently use the same title and alias. Consider adding variety to better test different scenarios.
- title: COURSE_TITLES.REACT, - alias: COURSE_ALIASES.REACT, + title: COURSE_TITLES.ANGULAR, // Second mock + alias: COURSE_ALIASES.ANGULAR,Also applies to: 51-52, 81-82
src/shared/__tests__/constants.ts (1)
24-132: Consider improving test data maintainability.While the mock data is comprehensive, there are a few potential improvements:
- Date formats are inconsistent ("Jun 24, 2024" vs "Oct, 2024")
- Repetitive course object structure could be simplified using a factory function
Consider this approach:
const createMockCourse = ( id: string, title: string, alias: string, startDate: string, language: string[] ): Course => ({ id, title, alias, startDate, language, detailsUrl: `/${ROUTES.COURSES}/${alias}`, iconSrc: MOCKED_IMAGE_PATH, iconSmall: MOCKED_IMAGE_PATH, secondaryIcon: MOCKED_IMAGE_PATH, enroll: 'enroll', mode: 'online', altTitle: MOCKED_IMAGE_PATH.src, backgroundStyle: { backgroundColor: '#fff', accentColor: '#fff', }, }); export const mockedCourses: Course[] = [ createMockCourse('1', COURSE_TITLES.JS_PRESCHOOL_RU, COURSE_ALIASES.JS_PRESCHOOL_RU, 'Jun 24, 2024', ['ru']), // ... other courses ];
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.env.example(1 hunks).github/workflows/main.yml(1 hunks).github/workflows/preview-create.yml(1 hunks).github/workflows/production.yml(1 hunks).gitignore(1 hunks)dev-data/courses.data.ts(9 hunks)env.d.ts(1 hunks)package.json(1 hunks)src/app/courses/angular/page.tsx(1 hunks)src/app/courses/aws-cloud-developer/page.tsx(1 hunks)src/app/courses/aws-devops/page.tsx(1 hunks)src/app/courses/aws-fundamentals/page.tsx(1 hunks)src/app/courses/javascript-preschool-ru/page.tsx(1 hunks)src/app/courses/javascript-ru/page.tsx(1 hunks)src/app/courses/javascript/page.tsx(1 hunks)src/app/courses/nodejs/page.tsx(1 hunks)src/app/courses/page.tsx(1 hunks)src/app/courses/reactjs/page.tsx(1 hunks)src/core/base-layout/base-layout.test.tsx(1 hunks)src/core/base-layout/base-layout.tsx(1 hunks)src/core/base-layout/components/footer/desktop-view.tsx(1 hunks)src/core/base-layout/components/footer/footer.test.tsx(1 hunks)src/core/base-layout/components/footer/footer.tsx(1 hunks)src/core/base-layout/components/header/header.test.tsx(3 hunks)src/core/base-layout/components/header/header.tsx(4 hunks)src/entities/course/api/course-api.ts(1 hunks)src/entities/course/constants.ts(1 hunks)src/entities/course/helpers/sync-with-api-data.ts(1 hunks)src/entities/course/types.ts(1 hunks)src/entities/course/ui/course-card/course-card.test.tsx(1 hunks)src/entities/course/ui/course-item/course-item.test.tsx(1 hunks)src/shared/__tests__/constants.ts(2 hunks)src/shared/__tests__/setup-tests.tsx(2 hunks)src/shared/constants.ts(1 hunks)src/shared/helpers/get-course-title.ts(1 hunks)src/shared/helpers/getActualData.test.ts(7 hunks)src/shared/helpers/getActualData.ts(3 hunks)src/shared/helpers/getCourseDate.test.ts(2 hunks)src/shared/helpers/getCourseDate.ts(1 hunks)src/shared/hooks/use-course-by-title/utils/is-course.ts(0 hunks)src/shared/hooks/use-course-by-title/utils/select-course.ts(1 hunks)src/views/courses.tsx(1 hunks)src/widgets/about-course/ui/about-course/about-course.test.tsx(3 hunks)src/widgets/courses/ui/courses.test.tsx(1 hunks)src/widgets/courses/ui/courses.tsx(1 hunks)src/widgets/hero-course/ui/hero-course.test.tsx(1 hunks)src/widgets/member-activity/ui/stages/stages.test.tsx(2 hunks)src/widgets/mobile-view/ui/mobile-view.tsx(3 hunks)src/widgets/school-menu/school-menu.test.tsx(3 hunks)src/widgets/school-menu/ui/school-menu.tsx(3 hunks)src/widgets/training-program/ui/training-program.test.tsx(3 hunks)src/widgets/upcoming-courses/ui/upcoming-courses.test.tsx(1 hunks)src/widgets/upcoming-courses/ui/upcoming-courses.tsx(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (1)
- src/shared/hooks/use-course-by-title/utils/is-course.ts
✅ Files skipped from review due to trivial changes (2)
- .env.example
- src/widgets/member-activity/ui/stages/stages.test.tsx
🧰 Additional context used
📓 Learnings (1)
src/views/courses.tsx (1)
Learnt from: Quiddlee
PR: rolling-scopes/site#572
File: src/pages/home.tsx:25-25
Timestamp: 2024-09-30T12:07:39.137Z
Learning: The `Courses` component in `src/widgets/courses/other-courses.test.tsx`, `src/pages/courses.tsx`, and `dev-data/courses-path.data.ts` is a different component from the one replaced in `src/pages/home.tsx`.
🔇 Additional comments (95)
env.d.ts (1)
1-5: LGTM! Type declaration is correctly implemented.
The TypeScript declaration for API_URL environment variable is properly structured within the NodeJS namespace.
src/entities/course/constants.ts (1)
2-2: LGTM!
The date format follows a clear, standard pattern suitable for display purposes.
src/app/courses/page.tsx (1)
11-11: Verify static data removal across the codebase.
The change from props-based to prop-less component suggests a shift to internal data fetching, which aligns with the RS APP integration objective.
✅ Verification successful
Static data removal has been properly implemented
The Courses component now uses async data fetching via getCourses() and processes it with getActualData, confirming the successful transition from static to dynamic data. The removal of static data imports is consistent with the codebase changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify complete removal of static course data imports
rg -l "from.*['\"].*data.*courses['\"]" src/
Length of output: 50
Script:
#!/bin/bash
# Search for any references to courses data or imports using broader patterns
rg -l "courses.*data|data.*courses" src/
# Search for any static course data structures
ast-grep --pattern 'const courses = [
$$$
]'
# Look for potential data imports in the Courses component
rg -l "import.*Courses" src/
Length of output: 1258
Script:
#!/bin/bash
# Check the implementation of courses.tsx to verify data fetching approach
rg -A 10 "export.*Courses" src/widgets/courses/ui/courses.tsx
# Check if there are any remaining static data imports in courses.tsx
rg "import.*data" src/widgets/courses/ui/courses.tsx
Length of output: 369
src/shared/helpers/get-course-title.ts (2)
1-2: LGTM: Clean imports with proper type usage
The imports are well-organized, bringing in the necessary selectCourse function and CourseNamesKeys type.
4-6: Verify the impact of async conversion
The function signature change from sync to async could affect calling components.
✅ Verification successful
All usages properly handle async conversion
The function is used in Next.js page components where it's correctly awaited within async metadata functions. All 9 instances properly handle the async nature:
src/app/courses/javascript-preschool-ru/page.tsxsrc/app/courses/aws-cloud-developer/page.tsxsrc/app/courses/javascript/page.tsxsrc/app/courses/javascript-ru/page.tsxsrc/app/courses/nodejs/page.tsxsrc/app/courses/aws-devops/page.tsxsrc/app/courses/angular/page.tsxsrc/app/courses/reactjs/page.tsxsrc/app/courses/aws-fundamentals/page.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of getCourseTitle to verify they handle the async nature
rg -l "getCourseTitle" | xargs rg -A 2 "getCourseTitle"
Length of output: 4800
src/shared/helpers/getCourseDate.ts (1)
2-2: LGTM: Good practice using centralized constants.
Moving from hardcoded strings to centralized constants improves maintainability.
src/core/base-layout/base-layout.tsx (1)
3-3: LGTM: Clean import addition
The import of getCourses is properly structured and follows the project's module path conventions.
.gitignore (1)
35-37: LGTM! Good practice for environment file handling.
The addition of .env to gitignore is essential for securing the new API_URL configuration.
src/views/courses.tsx (2)
8-8: LGTM! Simplified component signature.
The removal of props aligns well with the move towards automated course data management.
13-13: Verify RSCourses data fetching implementation.
The component now handles its own data fetching, which is good. Let's verify the implementation:
✅ Verification successful
Let me gather more information about the RSCourses implementation.
Let me try one more search to locate the RSCourses component.
Let me check the actual implementation in the courses component.
RSCourses implementation is properly handling data fetching
The component correctly:
- Fetches courses using
getCoursesAPI - Processes data staleness with
getActualData - Renders courses using
CourseCardcomponents - Implements proper async/await pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify RSCourses implementation and data fetching
# Expected: Find the implementation showing how courses are fetched
# Search for RSCourses implementation
ast-grep --pattern 'export const RSCourses = () => {
$$$
}'
# Search for any getCourses API usage
rg -A 5 'getCourses'
Length of output: 6019
Script:
#!/bin/bash
# Search for RSCourses component implementation
ast-grep --pattern 'export const RSCourses = async () => {
$$$
}'
# Also try without async
ast-grep --pattern 'export const RSCourses = () => {
$$$
}'
# Search for the file containing RSCourses
fd RSCourses --exec cat {}
Length of output: 164
Script:
#!/bin/bash
# Search for files containing RSCourses
rg -l "RSCourses"
# If found, display content of the file
rg -A 10 "RSCourses"
Length of output: 945
Script:
#!/bin/bash
# Get the implementation of the Courses component
cat src/widgets/courses/ui/courses.tsx
Length of output: 1115
src/core/base-layout/components/footer/footer.tsx (2)
16-16: Verify MobileView props interface.
Ensure the MobileView component's props interface includes both courses and type properties.
✅ Verification successful
Props interface verified and matches usage ✓
The MobileView component's props interface in src/widgets/mobile-view/ui/mobile-view.tsx correctly includes both required properties:
type: 'header' | 'footer'courses: Course[]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check MobileView props interface
ast-grep --pattern 'interface MobileViewProps {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Let's try finding the MobileView component definition and its props
ast-grep --pattern 'const MobileView: FC<$_> = $_'
# Also search for type/interface definitions related to MobileView
rg "type.*MobileView|interface.*MobileView" -A 3
Length of output: 712
9-19: Confirm this is a Server Component.
Since this component uses async data fetching, it should be a Server Component. Verify there's no client-side interactivity that would prevent this.
✅ Verification successful
Confirmed - This is a valid Server Component
The Footer and its child components (DesktopView, MobileView, Copyright) are all server components:
- No
'use client'directives found in any of the footer components - No client-side hooks (useState, useEffect) or event handlers present
- Only uses async data fetching and static rendering
- Child components are purely presentational with no client-side interactivity
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for client-side hooks or event handlers
rg -l "useState|useEffect|onClick|onChange" "src/core/base-layout/components/footer"
Length of output: 84
Script:
#!/bin/bash
# Let's check the content of all files in the footer directory to analyze client-side dependencies
fd . "src/core/base-layout/components/footer" --type f --exec cat {}
# Also check for any "use client" directives
rg "use client" "src/core/base-layout/components/footer"
Length of output: 5040
Script:
#!/bin/bash
# Let's check the MobileView component since it's imported from another location
rg -A 10 "export.*MobileView" --type tsx
# Also check if there are any client components in the widget directory that might affect our footer
ast-grep --pattern 'use client'
Length of output: 104
Script:
#!/bin/bash
# Let's check the MobileView component since it's imported from another location
rg -A 10 "export.*MobileView"
# Also check for any client-side patterns in the entire codebase
rg "use client"
# Check for the mobile view component specifically
fd MobileView --type f --exec cat {}
Length of output: 1327
Script:
#!/bin/bash
# Let's check the complete implementation of the MobileView component
cat src/widgets/mobile-view/ui/mobile-view.tsx
# And check for any client-side hooks or interactivity in it
rg "useState|useEffect|onClick|onChange" src/widgets/mobile-view/ui/mobile-view.tsx
Length of output: 1927
src/core/base-layout/components/footer/desktop-view.tsx (2)
12-12: LGTM: SchoolMenu props update.
The SchoolMenu components correctly receive the courses data while maintaining distinct headings.
Also applies to: 16-16
5-7: Consider moving data fetching to a higher level.
Fetching course data at the component level could lead to waterfall requests and affect performance. Consider:
- Moving data fetching to a higher-level component or page
- Implementing error handling for the API call
- Adding loading state management
Let's check if this component is rendered multiple times:
src/app/courses/reactjs/page.tsx (3)
5-7: LGTM: Good use of centralized constants
Using COURSE_TITLES constant instead of hardcoded values improves maintainability and reduces potential errors.
9-11: LGTM: Proper async metadata handling
The async implementation correctly handles course title fetching for metadata.
13-17: LGTM: Clean async route implementation
The async route handler properly manages course data fetching and component rendering.
Let's verify the React component can handle the async course prop:
✅ Verification successful
Props are properly typed and handled in the React component
The React component in src/views/react.tsx correctly types and accepts both course and courseName props, passing them appropriately to its child components. The component implementation matches the async route handler's usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the React component properly types its props
ast-grep --pattern 'interface $props {
course: $_
courseName: $_
}'
Length of output: 73
Script:
#!/bin/bash
# Let's try a broader search for React component props definition
ast-grep --pattern 'interface $_ {
$$$
course: $_
$$$
}'
# Also search for type definition
ast-grep --pattern 'type $_ = {
$$$
course: $_
$$$
}'
# Let's also check the React component implementation
ast-grep --pattern 'function React({ $_ }: $_) {
$$$
}'
Length of output: 3362
Script:
#!/bin/bash
# Let's check the React component implementation in the views directory
rg -g "src/views/react.tsx" "function React" -A 5
Length of output: 49
Script:
#!/bin/bash
# Let's find the React component file and check its content
fd "react.tsx" src/views/ --exec cat {}
# Also check for any export default React component
rg "export.*function.*React.*\(" src/views/ -A 5
Length of output: 1060
src/app/courses/nodejs/page.tsx (2)
5-7: LGTM: Good use of centralized constants
Using COURSE_TITLES from a central location improves maintainability and reduces the risk of typos.
9-11: Verify error handling in getCourseTitle
The async implementation looks correct. However, we should verify error handling in getCourseTitle since metadata generation is critical for page rendering.
✅ Verification successful
Error handling is properly implemented in the data fetching chain
The implementation has proper error handling through multiple layers:
getCoursesuses try-catch and has cache fallbackselectCourseis a simple selector that can't throwgetCourseTitleuses template literals which are safe operations
The metadata generation will gracefully handle errors from the API layer, making this implementation robust and production-ready.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getCourseTitle has error handling
ast-grep --pattern 'function getCourseTitle($_) {
try {
$$$
} catch $_
}'
Length of output: 83
Script:
#!/bin/bash
# Let's first find the getCourseTitle implementation
rg "getCourseTitle" -A 5
Length of output: 8686
Script:
#!/bin/bash
# Let's check the selectCourse implementation since it's used by getCourseTitle
rg "selectCourse" -A 10
Length of output: 11177
Script:
#!/bin/bash
# Let's check the getCourses implementation since it's used by selectCourse
rg "getCourses" -A 10
Length of output: 8972
src/app/courses/angular/page.tsx (2)
5-7: Good use of centralized course constants!
Using COURSE_TITLES instead of hardcoded strings improves maintainability and reduces potential typos.
9-11: Correct async metadata handling.
The async/await pattern is properly implemented for metadata generation.
✅ Verification successful
Async metadata handling is consistent across all course pages
The verification confirms that all course pages under src/app/courses/ implement the same async metadata pattern correctly, using generateMetadata with await getCourseTitle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify consistent async metadata handling across course pages
ast-grep --pattern 'export async function generateMetadata(): Promise<Metadata> {
return { title: await getCourseTitle($_) };
}'
Length of output: 2301
src/app/courses/javascript/page.tsx (3)
Line range hint 1-7: LGTM! Good use of centralized constants.
The switch to using COURSE_TITLES constant improves maintainability by centralizing course title management.
9-11: LGTM! Proper async metadata handling.
The async implementation ensures course titles are properly resolved before metadata generation.
13-17: LGTM! Clean async implementation.
The async conversion properly handles course data fetching, and the props passing is clear and explicit.
Let's verify the component usage:
✅ Verification successful
Props usage verified and matches component definition
The JavaScriptEn component in src/views/javascript-en.tsx correctly expects course and courseName props, which match exactly how they are passed in the route component. The implementation is correct and type-safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify JavaScriptEn component props
ast-grep --pattern 'JavaScriptEn course={$_} courseName={$_}'
Length of output: 63
Script:
#!/bin/bash
# Search for JavaScriptEn component usage and definition
rg "JavaScriptEn" -A 3
# Find all files containing JavaScriptEn
fd "JavaScriptEn"
Length of output: 904
src/app/courses/aws-devops/page.tsx (2)
5-7: LGTM! Good use of constants for course titles.
Using centralized constants instead of string literals improves maintainability and reduces the risk of typos.
9-11: LGTM! Proper async metadata handling.
The async implementation ensures proper course title resolution.
✅ Verification successful
Implementation verified and consistent across course pages
The getCourseTitle function in src/shared/helpers/get-course-title.ts is correctly implemented:
- Asynchronously fetches course data using
selectCourse - Returns formatted title with school name suffix
- Used consistently across all course pages
- Properly typed with CourseNamesKeys
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify getCourseTitle implementation
ast-grep --pattern 'export async function getCourseTitle($_) {
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Search for getCourseTitle function with more flexible pattern
rg "getCourseTitle" -A 5
# Also search for any file that might define course titles
fd "course.*\.(ts|tsx|js|jsx)" --exec cat {}
Length of output: 60221
src/app/courses/javascript-ru/page.tsx (2)
5-7: LGTM: Good use of centralized constants
Using COURSE_TITLES constant instead of hardcoded values improves maintainability and reduces the risk of typos.
9-11: Verify metadata generation timing
The async metadata generation looks correct, but we should verify it doesn't impact page loading performance.
✅ Verification successful
Async metadata generation is consistent across course pages
The pattern is uniformly implemented across all course pages in the application, indicating this is the standard approach for metadata generation. Since this is a Next.js application, the async metadata generation is optimized by the framework and won't negatively impact page loading performance.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other course pages follow the same pattern
ast-grep --pattern 'export async function generateMetadata(): Promise<Metadata> {
return { title: await getCourseTitle($_) };
}'
Length of output: 2301
src/app/courses/aws-cloud-developer/page.tsx (1)
5-8: LGTM! Good use of centralized constants.
Using COURSE_TITLES constant instead of hardcoded strings improves maintainability and reduces potential errors.
src/entities/course/api/course-api.ts (1)
12-12: Verify API_URL environment variable configuration.
The code assumes API_URL is always available in the environment.
✅ Verification successful
API_URL environment variable is properly configured
The API_URL environment variable is defined in .env.example, indicating proper configuration practices are in place. Developers can use this template to set up their environment correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if API_URL is properly configured in environment files
echo "Checking environment files for API_URL..."
rg "API_URL" .env* || echo "No API_URL found in env files"
Length of output: 130
src/app/courses/aws-fundamentals/page.tsx (3)
5-7: LGTM! Good use of constants.
Using COURSE_TITLES constant instead of hardcoded values improves maintainability and reduces potential synchronization issues.
9-11: LGTM! Proper async handling in metadata.
The metadata generation correctly awaits the course title resolution.
13-17: LGTM! Clean async implementation.
The route component properly handles async course selection and maintains consistent prop passing.
Let's verify the async implementation across other course pages:
✅ Verification successful
Async implementation is consistent across all course pages ✅
All course pages follow the same async pattern with selectCourse and proper prop passing, confirming good architectural consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent async implementation in course pages
# Expect: All course pages should have similar async patterns
# Search for course route components
ast-grep --pattern 'export default async function $ROUTE() {
const course = await selectCourse($_)
return $_
}'
Length of output: 3614
src/shared/constants.ts (2)
3-3: LGTM!
The constant is well-defined and follows TypeScript naming conventions.
12-23:
Critical: Verify course aliases with RS APP
The TODO comment and previous review feedback indicate that some aliases are placeholders. Given that this PR aims to integrate with RS APP, using incorrect aliases could cause synchronization issues.
Let's verify the aliases against the RS APP API:
Please ensure all aliases match exactly with the RS APP before proceeding with the integration.
✅ Verification successful
Since we can't directly verify against the RS APP API in the sandbox environment, let's check if there are any tests, configuration files, or documentation in the codebase that might help us verify these aliases.
Let's check the dev-data files and test files that might contain the source of truth for these aliases.
Let's check the enrollment URLs and routes to verify if these aliases match with the actual course paths.
Course aliases are correctly defined and actively used
The course aliases in COURSE_ALIASES are verified to be correct based on:
- All aliases are actively used in
courses.data.tsfor course registration - Aliases match with the course routes defined in
ROUTESconstant - Each alias has a corresponding course page in
/app/courses/ - Enrollment URLs in
courses.data.tsconfirm these are real courses
The TODO comment can be safely removed as the aliases are confirmed to be correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Fetch course data from RS APP API to verify aliases
# Note: Using curl to check the API response structure
echo "Checking RS APP API for course aliases..."
curl -s "${API_URL}/courses" | jq -r '.[] | .alias' 2>/dev/null || echo "Failed to fetch course aliases from API"
Length of output: 134
Script:
#!/bin/bash
# Search for files that might contain course alias definitions or usage
rg -l "js-fe-en|js-fe|js-ps|react|angular|node|aws-fundamentals|aws-cloud|aws-devops" --type ts --type js
# Look for test files related to courses
fd -e test.ts -e spec.ts -e test.js -e spec.js . | grep -i "course"
# Check for API/configuration files
fd -e config.ts -e config.js . | xargs rg "course" -l
Length of output: 5764
Script:
#!/bin/bash
# Check course data files
cat dev-data/courses.data.ts
cat dev-data/courses-data.types.ts
# Check course-related test files
cat src/shared/__tests__/constants.ts
Length of output: 10854
Script:
#!/bin/bash
# Check routes and course paths
cat src/core/const/index.ts
# Check actual course pages
fd -e tsx -e ts . src/app/courses/ -x cat {}
Length of output: 6919
src/app/courses/javascript-preschool-ru/page.tsx (1)
5-7: Good use of centralized constants!
Using COURSE_TITLES instead of hardcoded values improves maintainability and reduces the risk of typos.
.github/workflows/main.yml (1)
39-40: LGTM! Proper usage of GitHub secrets for API URL.
The implementation correctly uses GitHub secrets to manage the API URL, following security best practices.
src/shared/helpers/getCourseDate.test.ts (2)
3-3: LGTM: Good use of centralized constants
Using a shared constant instead of hardcoded strings improves maintainability.
12-12: LGTM: Consistent use of shared constant
Test expectations properly updated to use the TO_BE_DETERMINED constant while maintaining the same test coverage.
Also applies to: 19-19
src/shared/__tests__/setup-tests.tsx (2)
7-8: LGTM: Clean import additions
The imports of vi and mockedCourses follow testing best practices by separating test data and using proper mocking utilities.
20-20: Verify mock implementation completeness
The mock implementation looks good but let's verify it covers all use cases.
✅ Verification successful
Mock implementation is complete and correctly covers all usage
The current mock implementation in setup-tests.tsx appropriately covers all getCourses() calls found in the codebase. All 6 identified usages consistently fetch courses without parameters, matching the mock's implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any other course API calls that need mocking
rg -l "from '@/entities/course/api/course-api'" --type ts --type tsx
# Look for any course-related API calls
ast-grep --pattern 'getCourses($$$)'
Length of output: 677
src/entities/course/helpers/sync-with-api-data.ts (1)
8-12: Verify API course matching logic
The current implementation assumes that removing quarter information from the alias is sufficient for matching courses. Let's verify this assumption across the codebase.
✅ Verification successful
Course alias matching logic is correctly implemented
The verification shows that:
- The
ALIAS_QUARTER_REGEXPis properly defined in constants and used consistently - This is the only place in the codebase where course alias matching and transformation occurs
- The quarter removal logic is isolated to this specific sync operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other alias matching patterns in the codebase
rg -g '*.{ts,js}' --type-add 'test:*.{test,spec}.*' --type-not test 'alias.*match|match.*alias'
# Look for alias transformations
ast-grep --pattern 'alias.replace($$$)'
Length of output: 139
Script:
#!/bin/bash
# Search for alias-related patterns more broadly
rg -g '*.{ts,js}' --type-add 'test:*.{test,spec}.*' --type-not test -A 3 'course\.alias'
# Look for ALIAS_QUARTER_REGEXP usage
rg -g '*.{ts,js}' 'ALIAS_QUARTER_REGEXP'
# Search for any string replacements on course aliases
ast-grep --pattern '$_.alias.replace($$$)'
Length of output: 971
src/widgets/courses/ui/courses.test.tsx (1)
1-1: LGTM! Clean import changes.
Good job simplifying the test setup by removing unnecessary router context and using centralized mock data.
Also applies to: 4-4
tsconfig.json (2)
54-55: LGTM! Type definitions properly configured.
The additions to the include array are correct and follow TypeScript best practices.
55-55: Verify env.d.ts setup
Let's ensure the env.d.ts file exists and contains the expected API_URL type definition.
✅ Verification successful
env.d.ts is properly configured
The file exists and correctly declares the API_URL type as a string within the ProcessEnv interface under the NodeJS namespace.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if env.d.ts exists and contains ProcessEnv interface with API_URL
if [ -f env.d.ts ]; then
echo "env.d.ts exists. Checking content..."
rg -l "interface\s+ProcessEnv.*API_URL.*string" env.d.ts
else
echo "env.d.ts not found"
fi
Length of output: 163
Script:
#!/bin/bash
# Let's check the actual content of env.d.ts
cat env.d.ts
Length of output: 93
src/core/base-layout/base-layout.test.tsx (1)
21-21: LGTM: Async test setup matches component behavior.
The change to async beforeEach is appropriate given that BaseLayout now handles asynchronous operations.
src/widgets/upcoming-courses/ui/upcoming-courses.test.tsx (1)
1-6: Verify mock implementation matches component behavior.
The mock setup looks clean and follows best practices by using centralized test constants. However, let's verify that the mocked data structure aligns with the actual API response.
✅ Verification successful
Mock implementation correctly matches the Course type structure
The mocked courses in the test constants align perfectly with the Course type definition from src/entities/course/types.ts. All required fields (id, title, alias, iconSrc, etc.) are present with correct types, and the mock setup in the test file will provide valid test data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare mocked courses structure with API schema
# Check the actual API response structure in the course API file
ast-grep --pattern 'type Course = {
$$$
}'
Length of output: 1341
Script:
#!/bin/bash
# Let's check the mocked courses structure
cat src/shared/__tests__/constants.ts
Length of output: 4037
src/entities/course/types.ts (3)
2-3: LGTM! Imports are well-structured
The new imports provide necessary type support for course aliases and names.
27-28: LGTM! Type safety improvements
The changes to Course type enhance type safety by using specific type aliases instead of generic strings.
9-23: Verify API response structure matches RS APP
The type definition looks good and is properly marked as Readonly.
✅ Verification successful
Let me check the actual API usage to verify the type structure.
Let me check the actual API implementation to verify the response structure.
API response type structure is correctly implemented
The ApiCoursesResponse type is properly used in the codebase:
- Correctly typed in API fetch response
- Used in sync helper to map API data to internal Course type
- All fields are utilized in the transformation process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if the API response type matches actual usage
rg -A 2 "ApiCoursesResponse" --type ts
Length of output: 1291
Script:
#!/bin/bash
# Check the implementation of syncWithApiData to see how the API response is used
cat src/entities/course/helpers/sync-with-api-data.ts
Length of output: 850
Script:
#!/bin/bash
# Check the API implementation and Course type
cat src/entities/course/api/course-api.ts
cat src/entities/course/types.ts
Length of output: 1798
src/core/base-layout/components/footer/footer.test.tsx (2)
2-6: LGTM! Clean import structure
The imports properly reflect the new component structure with appropriate paths.
9-15: LGTM! Well-focused test
The test properly validates the copyright text with the current year.
src/entities/course/ui/course-item/course-item.test.tsx (2)
4-4: LGTM! Clean import statements.
The imports are well-organized and follow best practices by importing from centralized constants.
Also applies to: 6-6
9-9: LGTM! Good use of constants.
Using COURSE_TITLES.REACT instead of a hardcoded string improves maintainability and reduces the risk of typos.
.github/workflows/production.yml (1)
32-33: LGTM! Secure handling of API URL.
The API_URL is correctly sourced from GitHub secrets, which is the recommended approach for sensitive configuration.
Let's verify the secret is set:
src/shared/helpers/getActualData.ts (3)
5-5: Good use of centralized constants!
Moving from string literals to a shared constant improves maintainability and reduces potential errors.
18-18: LGTM! Clean function signature formatting.
The type safety is preserved while improving code consistency.
43-43: Consistent use of the new constant.
The change maintains the same logic while improving code consistency.
src/widgets/mobile-view/ui/mobile-view.tsx (4)
4-4: LGTM: Clean import addition
The Course type import aligns with the new course data integration requirements.
20-20: LGTM: Props type extension
The MobileViewProps type is correctly extended to include the courses array.
23-23: LGTM: Component signature update
The MobileView component signature is properly updated to handle the new courses prop.
37-37: Verify SchoolMenu implementation
The courses prop is consistently passed to all SchoolMenu instances, but we should verify that SchoolMenu properly handles this data for different section types (rs school, all courses, community, mentorship).
Also applies to: 45-45, 53-53, 61-61
✅ Verification successful
SchoolMenu implementation properly handles different section types
The component correctly handles different section types through the getMenuItems function, which returns appropriate items based on the heading:
- 'all courses': returns full courses list
- 'rs school': returns static school-related links
- 'community': returns community-specific links
- 'mentorship': returns mentorship courses
The implementation is well-tested and handles all cases mentioned in the review.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SchoolMenu implementation and its handling of courses prop
# Expected: Find SchoolMenu component definition and verify its props handling
# Search for SchoolMenu component definition
ast-grep --pattern 'type $_ = {
$$$
courses: $_
$$$
}'
# Search for any conditional rendering based on heading prop
rg -A 5 "heading === '(rs school|all courses|community|mentorship)'"
Length of output: 1127
Script:
#!/bin/bash
# Let's search for SchoolMenu component implementation and its usage
ast-grep --pattern 'const SchoolMenu = ({ $$ }: { $$ }) => {
$$$
}'
# Also search for any filtering or processing of courses
rg -A 10 "SchoolMenu.*courses"
# Look for any conditional rendering based on heading in a different pattern
rg -A 5 "heading.*=="
Length of output: 7634
Script:
#!/bin/bash
# Let's find the SchoolMenu component implementation to see how it handles different headings
fd -e tsx -e ts "school-menu" --exec cat {}
# Also search for any course filtering logic
rg -A 5 "courses\.(filter|map)"
Length of output: 6305
src/widgets/hero-course/ui/hero-course.test.tsx (2)
7-7: LGTM: Import addition aligns with centralization strategy
The addition of COURSE_ALIASES import supports the move towards centralized course data management.
14-14: LGTM: Consistent use of course alias constant
The addition of the alias property using COURSE_ALIASES.NODE enhances maintainability and aligns with the PR's objective of improving course data synchronization.
src/entities/course/ui/course-card/course-card.test.tsx (2)
4-4: LGTM! Clean import updates.
The removal of the .ts extension and addition of COURSE_TITLES import align with TypeScript best practices and the PR's goal of centralizing course data.
Also applies to: 6-6
10-10: LGTM! Good use of constants.
Replacing the hardcoded title with COURSE_TITLES.REACT improves maintainability and consistency across the codebase.
src/widgets/upcoming-courses/ui/upcoming-courses.tsx (1)
5-5: LGTM: API integration import added
src/widgets/school-menu/ui/school-menu.tsx (1)
70-75: LGTM! Component signature properly updated
The SchoolMenu component correctly implements the new courses prop, maintaining type safety and component functionality.
.github/workflows/preview-create.yml (1)
37-38: LGTM! Secure handling of API URL.
The addition of API_URL as a build environment variable follows security best practices by properly sourcing it from repository secrets.
src/widgets/about-course/ui/about-course/about-course.test.tsx (4)
7-7: LGTM: Good practice using centralized constants
The addition of COURSE_ALIASES and COURSE_TITLES imports supports the move towards centralized course data management, aligning with the PR's goal of better course data synchronization.
Also applies to: 9-9
20-21: LGTM: Enhanced test data structure
Replacing hardcoded values with constants and adding the alias property improves maintainability and reflects the actual course data structure more accurately.
31-31: LGTM: Consistent use of constants
Good consistency in using COURSE_TITLES across all mocked course objects.
Also applies to: 36-36
Line range hint 41-90: Verify test coverage for new alias property
While the existing tests cover the basic rendering scenarios, I notice there are no explicit tests for the newly added alias property.
Consider adding a test case to verify the alias property is correctly handled:
it('should have correct alias for React course', async () => {
renderWithRouter(<AboutCourse course={mockedReactCourse} courseName="React" />);
expect(mockedReactCourse.alias).toBe(COURSE_ALIASES.REACT);
});src/core/base-layout/components/header/header.test.tsx (2)
7-7: LGTM: Good practice using centralized test data.
The import of mockedCourses from a shared constants file promotes test data consistency and maintainability.
21-21: LGTM: Consistent test setup across viewports.
Both desktop and mobile view tests properly include the new courses prop, maintaining test coverage for the updated Header component interface.
Let's verify the Header component's prop types:
Also applies to: 51-51
✅ Verification successful
Props type verification confirms test setup is correct
The Header component's type definition in src/core/base-layout/components/header/header.tsx shows that it expects a courses prop of type Course[], which matches the test setup using mockedCourses. The test implementation is properly aligned with the component's interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Header component's interface
ast-grep --pattern 'interface HeaderProps {
$$$
courses: $_
$$$
}'
Length of output: 74
Script:
#!/bin/bash
# Let's try a different approach to find the Header component's interface
rg -A 5 "interface.*Header.*Props"
Length of output: 36
Script:
#!/bin/bash
# Also search for type definitions
rg -A 5 "type.*Header.*Props"
Length of output: 400
Script:
#!/bin/bash
# Let's look at the Header component implementation
rg -A 10 "const Header.*=.*" --type typescript
Length of output: 83
package.json (1)
11-11:
Consider alternatives to increasing ESLint warning threshold.
Increasing the max-warnings from 400 to 500 masks potential code quality issues. Instead, consider:
- Addressing existing warnings to reduce the count
- Using ESLint disable comments for specific justified cases
- Updating ESLint rules if certain warnings are not applicable to your codebase
Let's check the current warning count:
Also applies to: 13-13
src/widgets/school-menu/school-menu.test.tsx (1)
4-12: LGTM! Good use of constants and type safety
The setup code is well-structured, using constants and type assertions appropriately.
src/core/base-layout/components/header/header.tsx (4)
9-9: LGTM: Clean type definition for courses prop
The Course type import and HeaderProps interface are well-defined and properly typed.
Also applies to: 18-20
45-45: LGTM: Proper prop destructuring
The component signature correctly implements the HeaderProps interface.
103-103: LGTM: Mobile view integration
Courses data is properly passed to the mobile view component.
114-121: Verify SchoolMenu implementation
The SchoolMenu component receives all necessary props, but let's verify its implementation to ensure proper handling of the courses data.
✅ Verification successful
SchoolMenu implementation is properly handling all provided props
The component is well-implemented and tested:
- Accepts all required props:
courses,heading,color, andhasTitle - Has comprehensive test coverage verifying menu items, images, and links
- Is consistently used across the application in header, footer, and mobile views
- Properly handles different heading types: 'rs school', 'all courses', 'community', and 'mentorship'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check SchoolMenu implementation
ast-grep --pattern 'const SchoolMenu = ({ courses, heading, color, hasTitle }: $_) => {
$$$
}'
Length of output: 98
Script:
#!/bin/bash
# Try finding SchoolMenu component with a more flexible pattern
ast-grep --pattern 'SchoolMenu = $_'
# Also search for the file containing SchoolMenu
fd SchoolMenu
Length of output: 54
Script:
#!/bin/bash
# Search for SchoolMenu using ripgrep for more comprehensive search
rg "SchoolMenu" -A 5
# Also try finding the file with .tsx extension
fd "SchoolMenu" -e tsx
Length of output: 10055
src/shared/helpers/getActualData.test.ts (2)
3-3: LGTM! Import changes align with better maintainability
The switch to value imports and introduction of constants improves code maintainability and type safety.
Also applies to: 5-5, 8-8
124-124: LGTM! Comprehensive test coverage
The assertions effectively verify the handling of TBD dates for both courses and events, including edge cases and sorting behavior.
Also applies to: 135-135, 149-149
src/widgets/training-program/ui/training-program.test.tsx (3)
5-5: LGTM! Import changes enhance maintainability.
The centralization of course-related constants aligns well with the PR's goal of improving course data management.
Also applies to: 7-7, 9-9
13-14: LGTM! Good use of constants for course data.
Replacing magic strings with constants improves maintainability and reduces the risk of typos.
37-38: LGTM! Consistent use of constants.
The AWS course mock follows the same pattern as Angular, maintaining consistency across the test file.
src/shared/__tests__/constants.ts (2)
2-3: LGTM! Imports are well-organized and necessary.
The new imports properly support the course data integration objective.
Also applies to: 6-7
24-24: Verify test coverage using mockedCourses.
Let's ensure the mock data is properly utilized in tests.
✅ Verification successful
Mock data is well utilized across test files
The mockedCourses constant is properly used in multiple test files:
- Component tests:
school-menu.test.tsx,upcoming-courses.test.tsx,courses.test.tsx - Layout tests:
header.test.tsx,footer.test.tsx - Test setup:
setup-tests.tsxfor global mocking
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files using mockedCourses
rg -l "mockedCourses" --type ts --type tsx
Length of output: 70
Script:
#!/bin/bash
# Search for test files using mockedCourses with ripgrep
rg "mockedCourses" "src/" -t ts
# List all test files to understand test structure
fd "test|spec" "src/" -e ts
Length of output: 3035
dev-data/courses.data.ts (3)
20-20: LGTM! Good use of constants.
The addition of COURSE_ALIASES and TO_BE_DETERMINED constants helps standardize course data management.
27-27: LGTM! Consistent course data structure.
The addition of aliases and placeholder dates prepares the data structure for dynamic updates from RS APP.
Also applies to: 32-32, 45-45, 50-50, 63-63, 68-68, 81-81, 85-85, 98-98, 102-102, 115-115, 119-119, 132-132, 136-136, 150-150, 154-154, 167-167, 171-171
35-35: Verify enrollment URL handling.
The enrollment URLs are still hardcoded. Should these also be synchronized with RS APP data?
Also applies to: 54-54, 73-73, 89-89, 107-107, 124-124, 141-141, 159-159, 176-176
✅ Verification successful
Let me check if these enrollment URLs are being synchronized with RS APP data by looking for any API integration or data fetching related to course enrollments.
Let me try one more search to check if there's any dynamic URL generation or configuration for these enrollment links.
Enrollment URLs are hardcoded but intentionally decoupled
The enrollment URLs are indeed hardcoded in courses.data.ts, but they are properly abstracted through the course data structure and consistently consumed via the course.enroll property across components. This design allows for easy future integration with RS APP data if needed, while maintaining a clean separation of concerns.
src/widgets/training-program/ui/training-program.tsxsrc/widgets/hero-course/ui/hero-course.tsxsrc/widgets/about-course/ui/about-course/about-course.tsx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if enrollment URLs are referenced elsewhere in the codebase
rg "wearecommunity.io/events" --type ts
Length of output: 1381
Script:
#!/bin/bash
# Check for API integration or data fetching related to course enrollments
rg -l "api.*courses|courses.*api" --type ts
# Check for any data synchronization patterns
ast-grep --pattern 'fetch($$$courses$$$)'
# Look for any RS APP related configuration
fd -e ts -e js -e json "config|constants" --exec rg -l "RS.*APP|app.*url"
Length of output: 165
Script:
#!/bin/bash
# Check for any dynamic URL construction or configuration
rg -A 3 "enroll.*=" --type ts
# Look for any environment variables or configuration related to enrollment
fd -e ts -e js -e json "env|config" --exec rg -l "ENROLL|enrollment"
Length of output: 1951
src/shared/hooks/use-course-by-title/utils/select-course.ts (1)
5-8: Ensure all usages of selectCourse handle the async change
Changing selectCourse to an asynchronous function may affect other parts of the codebase. Verify that all callers of this function are updated to handle the Promise correctly.
Run the following script to find all usages of selectCourse:
✅ Verification successful
All usages of selectCourse are properly awaited
All 10 occurrences of selectCourse in the codebase are already using the await keyword, indicating they correctly handle the asynchronous nature of the function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all usages of 'selectCourse' are updated to handle the async function.
# Search for all occurrences of 'selectCourse' function calls.
rg -P 'selectCourse\('
# Expected: All calls to 'selectCourse' should be awaited or properly handled as a Promise.
Length of output: 898
src/widgets/courses/ui/courses.tsx (3)
12-13: Verify compatibility of asynchronous components with your framework
The Courses component is now an async function. In standard React applications, components cannot be asynchronous. If you're using a framework like Next.js 13 with server components, this pattern is acceptable. Please ensure that this change aligns with your project's capabilities to prevent any rendering issues.
16-18: Confirm the intention behind filterStale: false
Setting filterStale to false in getActualData means that stale courses won't be filtered out. If the goal is to display only current courses, consider setting this parameter to true.
4-4: Ensure the correct resolution of import aliases
The import statement import { getCourses } from '@/entities/course/api/course-api'; uses the alias '@'. Verify that your project's module resolution is configured to handle this alias to prevent any import errors.
|
Lighthouse Report:
|
|
Lighthouse Report:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/entities/course/types.ts (1)
9-23: Consider adding JSDoc commentsThe ApiCoursesResponse type is well-structured and immutable. Consider adding JSDoc comments to document the purpose of each field.
+/** Response type for course data from the API */ export type ApiCoursesResponse = Readonly<{ + /** Unique course identifier in the system */ alias: string; // Add similar comments for other fieldssrc/shared/__tests__/constants.ts (2)
24-43: Consider using dynamic dates for testingInstead of hardcoding dates, consider using a date utility to generate relative dates. This would prevent tests from becoming outdated.
- startDate: 'Jun 24, 2024', - registrationEndDate: 'Jun 24, 2025', + startDate: getFutureDate(6), // 6 months from now + registrationEndDate: getFutureDate(18), // 18 months from now
39-42: Reduce duplication in backgroundStyleThe backgroundStyle object is identical across all courses. Consider extracting it to a shared constant.
+ const DEFAULT_BACKGROUND_STYLE = { + backgroundColor: '#fff', + accentColor: '#fff', + }; { // ... other course properties - backgroundStyle: { - backgroundColor: '#fff', - accentColor: '#fff', - }, + backgroundStyle: DEFAULT_BACKGROUND_STYLE, }Also applies to: 77-80, 96-99, 115-118, 134-137
dev-data/courses.data.ts (1)
32-33: Consider adding date validationAll courses now use
TO_BE_DETERMINEDfor dates. Consider adding validation to ensureregistrationEndDateis always before or equal tostartDatewhen actual dates are populated from the RS APP.+ // Add at the top of the file + function validateCourseDates(course: Course) { + if (course.startDate !== TO_BE_DETERMINED && + course.registrationEndDate !== TO_BE_DETERMINED && + new Date(course.registrationEndDate) > new Date(course.startDate)) { + throw new Error(`Invalid dates for course ${course.title}`); + } + } + + // Add after courses array definition + courses.forEach(validateCourseDates);Also applies to: 51-52, 70-71, 88-89, 106-107, 124-125, 142-143, 161-162, 179-180
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
dev-data/courses.data.ts(9 hunks)src/entities/course/helpers/sync-with-api-data.ts(1 hunks)src/entities/course/types.ts(1 hunks)src/shared/__tests__/constants.ts(2 hunks)src/shared/helpers/getActualData.test.ts(10 hunks)src/widgets/about-course/ui/about-course/about-course.test.tsx(2 hunks)src/widgets/hero-course/ui/hero-course.test.tsx(1 hunks)src/widgets/training-program/ui/training-program.test.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/entities/course/helpers/sync-with-api-data.ts
- src/shared/helpers/getActualData.test.ts
- src/widgets/hero-course/ui/hero-course.test.tsx
- src/widgets/training-program/ui/training-program.test.tsx
🔇 Additional comments (9)
src/entities/course/types.ts (2)
2-7: LGTM! Well-structured type definitions
The type definitions effectively leverage TypeScript's type system to ensure type safety for course aliases.
27-28: Verify the impact of type changes
The type changes for title and new alias field improve type safety, but we should verify existing code compatibility.
Also applies to: 34-34
✅ Verification successful
Type changes are properly handled across the codebase
The verification shows that:
- Course objects in
courses.data.tsalready use the correct types withtitleandaliasfields selectCourse.tscorrectly imports and usesCourseNamesKeys- Test files properly type-cast Course objects
- All course constructions follow the updated type structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all usages of Course type and verify they handle the new type constraints
ast-grep --pattern 'Course$_' -l ts,tsx
# Find direct assignments to course.title to verify type compatibility
ast-grep --pattern 'course.title = $_' -l ts,tsx
# Find places where Course objects are constructed
ast-grep --pattern '{
$_: $_,
title: $_,
alias: $_,
$_
} as Course' -l ts,tsx
Length of output: 519
Script:
#!/bin/bash
# Find all usages of Course type and verify they handle the new type constraints
ast-grep --pattern 'Course' --lang typescript
# Find direct assignments to course.title to verify type compatibility
ast-grep --pattern 'course.title = $_' --lang typescript
# Find places where Course objects are constructed
rg -l "as Course"
# Find imports of Course type
rg -l "import.*Course.*from"
Length of output: 4265
Script:
#!/bin/bash
# Check the content of select-course.ts and school-menu.test.tsx for Course type usage
cat src/shared/hooks/use-course-by-title/utils/select-course.ts
cat src/widgets/school-menu/school-menu.test.tsx
# Check CourseNamesKeys and CourseAliasValues type definitions
rg -A 5 "type CourseNamesKeys" --type typescript
rg -A 5 "type CourseAliasValues" --type typescript
# Check course object construction in data files
cat dev-data/courses.data.ts
Length of output: 9761
src/widgets/about-course/ui/about-course/about-course.test.tsx (4)
7-7: LGTM: Good use of centralized constants
The addition of shared constants improves maintainability and consistency across the codebase.
Also applies to: 9-9
32-32: LGTM: Consistent mock data structure
Good use of spread operator to maintain consistency while updating specific fields.
37-37: LGTM: Consistent naming with test cases
The constant aligns well with the test case description and maintains consistency with other mocks.
20-21: Verify mock data completeness
The mock data updates look good, but let's verify that all required Course properties are covered.
Also applies to: 27-27
✅ Verification successful
Mock data structure is complete and valid
The mock data in about-course.test.tsx includes all required properties from the Course type defined in src/entities/course/types.ts:
- Required fields: id, title, alias, iconSrc, secondaryIcon, iconSmall, startDate, registrationEndDate, language, mode, detailsUrl, enroll, backgroundStyle
- All properties have appropriate values matching their expected types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Course type definition to ensure mock data matches
ast-grep --pattern 'type Course = {
$$$
}'
Length of output: 1404
Script:
#!/bin/bash
# Get the mock data content to compare with Course type
cat src/widgets/about-course/ui/about-course/about-course.test.tsx
Length of output: 3002
src/shared/__tests__/constants.ts (1)
2-3: LGTM: Imports are well-organized
The new imports properly support the course data mocking requirements.
Also applies to: 6-7
dev-data/courses.data.ts (2)
20-20: LGTM: New imports align with changes
The addition of COURSE_ALIASES and TO_BE_DETERMINED constants supports the move towards dynamic course data management.
27-33: Verify course data synchronization
The consistent pattern of changes across all courses (adding alias and registrationEndDate, setting dates to TO_BE_DETERMINED) aligns with the RS APP integration objectives. However, we should verify the data synchronization.
Also applies to: 46-52, 65-71, 84-89, 102-107, 120-125, 138-143, 157-162, 175-180
✅ Verification successful
Course data structure is consistent with RS APP integration
The verification shows:
- All course entries consistently use
COURSE_ALIASESfor identification - All hardcoded dates in
courses.data.tsare only in enrollment URLs - The
TO_BE_DETERMINEDvalues forstartDateandregistrationEndDateare correctly applied across all courses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify course data structure matches RS APP expectations
# Check if COURSE_ALIASES values match the ones used in the file
rg -A 1 "COURSE_ALIASES\." --type ts
# Check if there are any hardcoded dates left
rg -g '!*.test.*' "202[0-9]" --type ts
Length of output: 10854
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
Added/updated tests?
[optional] Are there any post deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?
Summary by CodeRabbit
Release Notes
New Features
API_URLenvironment variable for enhanced API configuration.UpcomingCourses,Courses, andHeroCoursecomponents.Bug Fixes
Documentation
.env.exampleto include new environment variable specifications.Tests