871-refactor: Refactor youtube api #872
Conversation
github-actions
bot
changed the title
Quiddlee
requested review from
SpaNb4,
andron13,
dzmitry-varabei,
ansivgit and
natanchik
as code owners
📝 WalkthroughWalkthroughThe changes introduce a refactored YouTube API integration using a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MentorTalksVideoPlaylistWithPlayer
participant mentorStore
participant MentorApi
participant YouTubeAPI
User->>MentorTalksVideoPlaylistWithPlayer: Render component
MentorTalksVideoPlaylistWithPlayer->>mentorStore: loadYoutubePlaylist()
mentorStore->>MentorApi: queryMentorPlaylist()
MentorApi->>YouTubeAPI: GET /playlistItems
YouTubeAPI-->>MentorApi: Playlist data
MentorApi-->>mentorStore: MentorPlaylistResponse
mentorStore->>mentorStore: filter & transform videos
mentorStore-->>MentorTalksVideoPlaylistWithPlayer: Video[]
MentorTalksVideoPlaylistWithPlayer->>User: Render VideoPlaylistWithPlayer with videoItems
Assessment against linked issues
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 30th. To opt out, configure 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
🧰 Additional context used🧬 Code Graph Analysis (1)src/core/api/app-api.ts (2)
🔇 Additional comments (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 5
🧹 Nitpick comments (9)
src/shared/helpers/filter-youtube-private-videos.ts (2)
1-5: Add error handling for unexpected data structuresThe function properly filters videos based on privacy status, but doesn't handle possible edge cases.
export function filterYoutubePrivateVideos(videos: GoogleApiYouTubePlaylistItemResource[]) { - return videos.filter((item) => item.status.privacyStatus === videoPrivacyStatus.public); + return videos?.filter((item) => item?.status?.privacyStatus === videoPrivacyStatus.public) || []; }
1-5: Consider importing GoogleApiYouTubePlaylistItemResource typeThe type for the input parameter is used but not imported, which might make the code harder to understand without seeing the full type definition.
src/entities/mentor/helpers/transform-mentors-videos.ts (2)
3-9: Add error handling for unexpected data structuresThe transformation function doesn't handle cases where the input array might contain items with missing properties.
export function transformMentorsVideos(videos: GoogleApiYouTubePlaylistItemResource[]): Video[] { - return videos.map((item) => ({ - id: item.snippet.resourceId.videoId, - title: item.snippet.title, - thumbnail: item.snippet.thumbnails.medium.url, - })); + return videos?.map((item) => { + if (!item?.snippet) return null; + + return { + id: item.snippet?.resourceId?.videoId || '', + title: item.snippet?.title || '', + thumbnail: item.snippet?.thumbnails?.medium?.url || '', + }; + }).filter(Boolean) as Video[] || []; }
1-9: Consider importing GoogleApiYouTubePlaylistItemResource typeThe type for the input parameter is used but not imported, which would improve code readability and type checking.
src/entities/mentor/model/store.ts (1)
7-11: Consider enhancing error handling.The error message is generic and doesn't include details about the specific error, which might make debugging harder.
if (res.isSuccess) { return res.result; } -throw new Error('Error while loading mentor playlist.'); +throw new Error(`Error while loading mentor playlist: ${res.error || 'Unknown error'}`);.env.example (1)
2-3: Specific URL in example file may not be ideal.Using a specific URL (
https://cdn.rs.school) in the example file could lead developers to inadvertently use the production URL in development environments.-NEXT_PUBLIC_API_BASE_URL=https://cdn.rs.school +NEXT_PUBLIC_API_BASE_URL=https://example.api.urlsrc/core/api/app-api.ts (1)
12-15: Constructor parameters could be optimized.The parameters are marked as
private readonlybut are only used within the constructor.constructor( - private readonly baseURI: string, - private readonly youtubeBaseURI: string, + baseURI: string, + youtubeBaseURI: string, ) {src/entities/mentor/api/mentor-api.ts (1)
6-19: Good API abstraction with proper dependency injectionThe MentorApi class nicely encapsulates YouTube API interactions with:
- Clean dependency injection through constructor
- Well-defined method for playlist queries
- Appropriate use of environment variables for API key
- Proper typing with the MentorPlaylistResponse type
Consider adding error handling or request options (like timeout) to make the API calls more robust.
src/shared/ui/video-playlist-with-player/video-playlist-with-player.test.tsx (1)
23-64: Video selection test needs explanation for multiple VideoPlayer callsThe test expects VideoPlayer to be called 3 times, but it's not immediately clear why. Consider adding a comment explaining the sequence of calls:
- Initial render
- Re-render after state updates
- After clicking a different video
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
.env.example(1 hunks).github/workflows/main.yml(1 hunks).github/workflows/preview-create.yml(1 hunks).github/workflows/production.yml(1 hunks).github/workflows/visual-testing-on-comment.yml(1 hunks).github/workflows/visual-testing-on-push.yml(1 hunks)env.d.ts(1 hunks)package.json(1 hunks)src/core/api/app-api.ts(2 hunks)src/entities/mentor/api/mentor-api.ts(1 hunks)src/entities/mentor/constants.ts(1 hunks)src/entities/mentor/helpers/transform-mentors-videos.ts(1 hunks)src/entities/mentor/index.ts(1 hunks)src/entities/mentor/model/store.ts(1 hunks)src/entities/mentor/types.ts(1 hunks)src/shared/api/api-base-class.ts(2 hunks)src/shared/api/api.ts(1 hunks)src/shared/constants.ts(2 hunks)src/shared/helpers/filter-youtube-private-videos.ts(1 hunks)src/shared/helpers/log-request.ts(2 hunks)src/shared/types/types.ts(1 hunks)src/shared/ui/video-playlist-with-player/video-playlist-with-player.test.tsx(4 hunks)src/shared/ui/video-playlist-with-player/video-playlist-with-player.tsx(3 hunks)src/widgets/mentors-feedback/ui/mentors-feedback.tsx(2 hunks)src/widgets/mentors-feedback/ui/mentors-playlist/mentor-talks-video-playlist-with-player.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/shared/helpers/filter-youtube-private-videos.ts (1)
src/shared/ui/video-playlist-with-player/constants.ts (1)
videoPrivacyStatus(1-4)
src/widgets/mentors-feedback/ui/mentors-feedback.tsx (1)
src/widgets/mentors-feedback/ui/mentors-playlist/mentor-talks-video-playlist-with-player.tsx (1)
MentorTalksVideoPlaylistWithPlayer(10-29)
src/shared/api/api.ts (1)
src/core/api/app-api.ts (1)
Api(6-24)
src/entities/mentor/helpers/transform-mentors-videos.ts (2)
src/entities/mentor/index.ts (1)
transformMentorsVideos(4-4)src/shared/types/types.ts (1)
Video(12-16)
src/entities/mentor/model/store.ts (2)
src/shared/api/api.ts (1)
api(3-6)src/entities/mentor/index.ts (1)
mentorStore(3-3)
src/entities/mentor/api/mentor-api.ts (4)
src/shared/types/types.ts (1)
ApiServices(18-21)src/entities/mentor/types.ts (1)
MentorPlaylistResponse(10-12)src/shared/constants.ts (1)
YOUTUBE_API_MAX_RESULTS_PER_PAGE(10-10)src/entities/mentor/constants.ts (1)
MENTORSHIP_YOUTUBE_PLAYLIST_ID(1-1)
src/shared/ui/video-playlist-with-player/video-playlist-with-player.tsx (1)
src/shared/types/types.ts (1)
Video(12-16)
🔇 Additional comments (30)
src/shared/helpers/log-request.ts (2)
1-1: Use named import forinspect.
Importinginspectdirectly from'util'simplifies the code and enables tree-shaking in modern bundlers.
60-63: Direct invocation ofinspect.
Callinginspect(logObject, { depth: null, colors: process.env.NODE_ENV !== 'production' })is concise and maintains clear logging behavior.package.json (1)
49-50: Good addition of SWR for data fetchingAdding SWR as a dependency is appropriate for implementing client-side data fetching with caching in the mentor playlist component.
src/entities/mentor/constants.ts (1)
1-1: Good use of constants for YouTube playlist IDExtracting the YouTube playlist ID into a named constant improves maintainability by centralizing this configuration value.
src/shared/types/types.ts (1)
20-20: Good API service separationAdding a dedicated YouTube API service property improves code organization by separating different API clients.
src/entities/mentor/index.ts (1)
3-4: New exports for refactored YouTube APIThese exports expose the mentor store for state management and the transformation utility needed by the new mentor video playlist implementation.
.github/workflows/production.yml (1)
24-26:✅ Verification successful
Environment variables updated to follow Next.js conventions
The environment variables now follow the
NEXT_PUBLIC_prefix convention for client-side exposure.Ensure that other workflow files and environment configurations have been updated consistently to use these new variable names.
🏁 Script executed:
#!/bin/bash # Verify other workflow files are using the same env var naming convention grep -r "API_BASE_URL\|YOUTUBE_API_BASE_URL\|YOUTUBE_API_KEY" .github/workflows/ --include="*.yml" | grep -v "NEXT_PUBLIC_"Length of output: 123
🏁 Script executed:
#!/bin/bash # Verify that any .env* files have been updated to use NEXT_PUBLIC_ prefixes grep -R "API_BASE_URL\|YOUTUBE_API_BASE_URL\|YOUTUBE_API_KEY" -n --include="*.env*" . | grep -v "NEXT_PUBLIC_"Length of output: 110
No stale environment variable references detected
Workflows and env files were scanned—no occurrences of the oldAPI_BASE_URL,YOUTUBE_API_BASE_URL, orYOUTUBE_API_KEYwithout theNEXT_PUBLIC_prefix remain.src/shared/api/api-base-class.ts (2)
62-62: Default logging enabledThe
shouldLogparameter now defaults totrue, ensuring API requests are logged by default unless explicitly disabled.
125-125: URL construction simplifiedString concatenation is now used instead of the URL constructor to combine base URL and query string.
.github/workflows/main.yml (1)
12-14: Environment variables updated for Next.js client-side accessEnvironment variables have been renamed with the
NEXT_PUBLIC_prefix to align with Next.js conventions for client-side accessibility. Additional YouTube API variables have been added to support the new functionality.src/shared/api/api.ts (1)
3-6: API initialization updated to support multiple servicesThe
Apiclass is now initialized with two base URLs to support both the main API and YouTube API services, aligning with the updated constructor insrc/core/api/app-api.ts..github/workflows/preview-create.yml (1)
19-21: Consistent environment variable updatesEnvironment variables have been updated with the
NEXT_PUBLIC_prefix and additional YouTube API variables to maintain consistency across all workflows..github/workflows/visual-testing-on-push.yml (1)
10-12: Environment variables correctly updated with NEXT_PUBLIC_ prefixThe environment variables have been properly updated with the Next.js public environment variable convention (
NEXT_PUBLIC_prefix), which makes them accessible on the client side..github/workflows/visual-testing-on-comment.yml (1)
11-13: Environment variables correctly updated with NEXT_PUBLIC_ prefixThe environment variables have been properly updated with the Next.js public environment variable convention (
NEXT_PUBLIC_prefix), matching the changes made in other workflow files.src/shared/constants.ts (2)
10-10: Good addition of YouTube API pagination constantAdding a constant for the maximum results per page is a good practice for API pagination settings.
108-108: Appropriate SWR cache key constant addedThe SWR cache key constant helps maintain consistency in data fetching cache identifiers across components.
src/widgets/mentors-feedback/ui/mentors-feedback.tsx (2)
6-8: Clean import of specialized componentImport of the specialized playlist component aligns with the refactoring approach of separating concerns.
53-53: Improved component usage with encapsulated playlist fetchingThe change simplifies the component usage by encapsulating all YouTube playlist fetching and processing logic within the specialized component, improving readability and maintainability.
src/entities/mentor/model/store.ts (1)
1-15: Simple and focused store implementation.The store provides a clean abstraction for loading YouTube playlist data with proper error handling.
env.d.ts (1)
4-6: Proper environment variable declarations for Next.js client-side usage.The
NEXT_PUBLIC_prefix follows Next.js conventions for client-exposed variables..env.example (1)
5-7: Well-structured environment variables for YouTube API.The section is clearly marked and includes the standard YouTube API v3 endpoint.
src/core/api/app-api.ts (3)
16-19: Clean separation of API services.Creating separate service instances for REST and YouTube APIs promotes better separation of concerns.
10-10: Mentor API integration looks good.The MentorApi is properly integrated following the same pattern as the existing TrainerApi.
Also applies to: 22-22
1-24:✅ Verification successful
Verify YouTube API key handling.
It's not clear how the YouTube API key is passed to the ApiBaseClass.
🏁 Script executed:
#!/bin/bash # Check how the API key is handled in ApiBaseClass echo "ApiBaseClass implementation:" grep -A 20 "class ApiBaseClass" $(find src -name "api-base-class.ts") echo "YouTube API key usage:" grep -r "NEXT_PUBLIC_YOUTUBE_API_KEY" src --include="*.ts" --include="*.tsx"Length of output: 967
YouTube API key is handled in MentorApi
The
ApiBaseClassdoesn’t need the key in its constructor—mentor-api.tsadds it as a query parameter (key: process.env.NEXT_PUBLIC_YOUTUBE_API_KEY). No changes required inapp-api.ts.src/widgets/mentors-feedback/ui/mentors-playlist/mentor-talks-video-playlist-with-player.tsx (1)
1-29: Well-architected component with clean separation of concernsThis new component follows best practices by:
- Using SWR for data fetching with appropriate cache key
- Properly handling loading and error states with testable UI elements
- Filtering private videos and transforming data before passing to the presentation component
- Keeping the component focused on its primary responsibility
The implementation effectively encapsulates the fetching and processing logic that was previously mixed with presentation concerns.
src/shared/ui/video-playlist-with-player/video-playlist-with-player.test.tsx (2)
15-21: Simplified test correctly matches component changesThe test now properly checks the component with directly provided props instead of mocking fetch calls, which aligns with the refactored component's design.
66-94: Improved test for playlist height with more direct DOM manipulationThe test now directly manipulates the player element's offsetHeight property rather than using a global prototype override, which is a safer approach.
src/shared/ui/video-playlist-with-player/video-playlist-with-player.tsx (3)
14-16: Props interface simplified for better component responsibilityRemoving the API fetching props in favor of direct video items improves component responsibility separation.
22-36: Effective use of callback ref for cleanupUsing a callback ref to handle both initial measurement and resize event cleanup is an elegant solution.
47-57: Improved JSX structure with better semanticsThe refactored JSX structure with explicit data-testid attributes improves testability.
src/widgets/mentors-feedback/ui/mentors-playlist/mentor-talks-video-playlist-with-player.tsx
Outdated
Show resolved
Hide resolved
|
Lighthouse Report:
|
|
Lighthouse Report:
|
|
@Quiddlee We should also update the env vars in the repo settings so CI/CD works correctly, right? upd: Never mind, I see you're mapping them to different names in the workflow files |
I have updated the env names only in our codebase, the ci/cd names should remain the same |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/shared/helpers/filter-youtube-private-videos.ts (1)
1-6: Function implementation looks good, but consider adding type safety improvements.The function correctly filters out private videos from the YouTube playlist response. For better maintainability, consider:
- Adding an explicit return type
- Adding null/undefined checks for defensive programming
- export function filterYoutubePrivateVideos(videos: MentorPlaylistResponse) { - return videos.items.filter((item) => item.status.privacyStatus === videoPrivacyStatus.public); + export function filterYoutubePrivateVideos(videos: MentorPlaylistResponse): GoogleApiYouTubePlaylistItemResource[] { + if (!videos?.items?.length) return []; + return videos.items.filter((item) => item.status?.privacyStatus === videoPrivacyStatus.public);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
env.d.ts(1 hunks)src/entities/mentor/helpers/transform-mentor-videos.ts(1 hunks)src/entities/mentor/index.ts(1 hunks)src/entities/mentor/model/store.ts(1 hunks)src/shared/helpers/filter-youtube-private-videos.ts(1 hunks)src/shared/helpers/log-request.ts(3 hunks)src/widgets/mentors-feedback/ui/mentors-playlist/mentor-talks-video-playlist-with-player.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/entities/mentor/helpers/transform-mentor-videos.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/entities/mentor/index.ts
- src/shared/helpers/log-request.ts
- src/widgets/mentors-feedback/ui/mentors-playlist/mentor-talks-video-playlist-with-player.tsx
- src/entities/mentor/model/store.ts
- env.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/shared/helpers/filter-youtube-private-videos.ts (2)
src/entities/mentor/types.ts (1)
MentorPlaylistResponse(10-12)src/shared/ui/video-playlist-with-player/constants.ts (1)
videoPrivacyStatus(1-4)
|
Lighthouse Report:
|
What type of PR is this? (select all that apply)
Description
Related Tickets & Documents
Screenshots, Recordings
Please replace this line with any relevant images for UI changes.
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
New Features
Improvements
NEXT_PUBLIC_prefix for better client-side exposure.Bug Fixes
Chores