Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request updates the markdown rendering logic by replacing dynamic imports of the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant R as Router
participant CP as Course Page (index.js)
participant MT as MenuTab
participant MS as MarkdownDisplay
participant CS as CourseSidebar
U->>R: Navigate to course page
R->>CP: Load course page component
CP->>CP: Initialize state (activeIndex, activeTab, sidebarVisible)
CP->>MT: Render MenuTab with sidebar toggle props
MT->>CP: Toggle sidebar on user interaction
CP->>CS: Conditionally render CourseSidebar (mobile view)
CP->>MS: Render course content via MarkdownDisplay
U-->>CP: Interact with tabs and content
sequenceDiagram
participant U as User
participant N as Navbar
participant ND as NDK Context
participant CH as CourseHeader
U->>N: Access course-related route
N->>ND: Fetch course data using context and decode slug
ND-->>N: Return course event data
N->>CH: Render CourseHeader with course details
CH->>U: Provide back navigation and responsive header UI
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (7)
✨ 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:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/components/content/courses/CourseHeader.js (1)
32-35: Improve keyboard accessibility for the clickable divThe div with onClick handler should be accessible to keyboard users. Consider using a button element or adding keyboard event handlers.
<div - className="flex items-center cursor-pointer hover:opacity-90" + className="flex items-center cursor-pointer hover:opacity-90" + role="button" + tabIndex={0} onClick={() => router.push(`/course/${router.query.slug}`)} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + router.push(`/course/${router.query.slug}`); + } + }} >src/components/content/courses/CourseSidebar.js (1)
141-142: Consider making the height calculation more flexible.The fixed height calculation (
h-[calc(100vh-400px)]) might be brittle if other layout elements change size. Consider using a more dynamic approach based on the actual size of other elements, or at least extract this calculation to a variable with a clear comment explaining the 400px offset.src/components/navbar/Navbar.js (4)
25-26: Inline state for course and completion status.
StoringcourseandisCompletedin local state is straightforward. However, if the codebase grows, consider encapsulating completion logic within a helper to reduce repeated condition checks throughout.
28-29: Potential route matching edge cases.
router.pathname.startsWith('/course/')works if no subpath ambiguity exists (e.g.,/course-like/). If unexpected collisions might occur, consider a more robust route check or Next.js route param usage.
31-68: Side effect fetch logic.
Fetching course data inuseEffectbased onisCoursePageand session info is sensible. A few considerations:
- The effect re-runs on every relevant dependency change (e.g.,
session?.user?.completedCourses). Ensure this won't cause redundant network calls if the session updates frequently.- Error handling currently logs to console; consider a user-facing fallback (e.g., a toast or a minimal UI feedback) if the fetch fails.
Otherwise, the flow looks solid.
117-142: Conditional branding vs. course header.
Conditionally displayingCourseHeaderis correct. If the course is still loading, though, users might see the standard branding briefly. Consider a loading state in the navbar or a placeholder if that flicker is undesirable.src/components/content/courses/CourseDetails.js (1)
45-46: Mobile detection fallback.
localIsMobileViewis a nice fallback if the parent doesn’t provideisMobileView. Just be mindful of duplicating logic (e.g., if there's a single source of truth in the app for mobile detection).src/pages/course/[slug]/index.js (1)
183-192: Auto-setting nAddress from slug.
FetchingnAddressif the slug includes'naddr'is consistent with your NIP-19 usage. Consider also handling slug validations (empty or incorrectly formatted) with user feedback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
package.json(2 hunks)src/components/content/combined/CombinedDetails.js(3 hunks)src/components/content/courses/CombinedLesson.js(3 hunks)src/components/content/courses/CourseDetails.js(4 hunks)src/components/content/courses/CourseHeader.js(1 hunks)src/components/content/courses/CourseLesson.js(3 hunks)src/components/content/courses/CourseSidebar.js(1 hunks)src/components/content/courses/DocumentLesson.js(3 hunks)src/components/content/courses/DraftCourseDetails.js(2 hunks)src/components/content/courses/DraftCourseLesson.js(2 hunks)src/components/content/courses/VideoLesson.js(3 hunks)src/components/content/documents/DocumentDetails.js(3 hunks)src/components/content/videos/VideoDetails.js(3 hunks)src/components/markdown/MarkdownDisplay.js(1 hunks)src/components/menutab/MenuTab.js(2 hunks)src/components/navbar/Navbar.js(3 hunks)src/components/profile/DataTables/UserProgressTable.js(0 hunks)src/config/appConfig.js(1 hunks)src/pages/course/[slug]/index.js(7 hunks)src/pages/draft/[slug]/index.js(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/profile/DataTables/UserProgressTable.js
🧰 Additional context used
🧬 Code Graph Analysis (14)
src/components/content/courses/DraftCourseLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/documents/DocumentDetails.js (2)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)src/pages/details/[slug]/index.js (1)
decryptedContent(22-22)
src/components/menutab/MenuTab.js (2)
src/pages/course/[slug]/index.js (2)
isMobileView(179-179)sidebarVisible(176-176)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)
src/components/content/combined/CombinedDetails.js (2)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)src/pages/details/[slug]/index.js (1)
decryptedContent(22-22)
src/components/content/videos/VideoDetails.js (2)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)src/pages/details/[slug]/index.js (1)
decryptedContent(22-22)
src/components/content/courses/DocumentLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/DraftCourseDetails.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/VideoLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/CourseDetails.js (3)
src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/zaps/ZapDisplay.js (1)
ZapDisplay(7-46)src/components/ui/MoreOptionsMenu.js (1)
MoreOptionsMenu(14-67)
src/components/content/courses/CourseSidebar.js (5)
src/components/content/courses/CourseDetails.js (1)
useImageProxy(40-40)src/components/navbar/Navbar.js (1)
navbarHeight(19-19)src/pages/course/[slug]/index.js (5)
navbarHeight(181-181)activeIndex(174-174)isMobileView(179-179)completedLessons(175-175)lessons(91-91)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)
src/components/content/courses/CombinedLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/CourseLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/pages/course/[slug]/index.js (3)
src/components/content/courses/CourseSidebar.js (2)
navbarHeight(20-20)CourseSidebar(7-152)src/components/ZapThreadsWrapper.js (1)
ZapThreadsWrapper(3-56)src/components/menutab/MenuTab.js (1)
MenuTab(6-45)
src/components/navbar/Navbar.js (2)
src/components/content/courses/CourseHeader.js (2)
router(12-12)CourseHeader(6-80)src/utils/nostr.js (2)
parseCourseEvent(149-218)parseCourseEvent(149-218)
🪛 Biome (1.9.4)
src/components/content/courses/CourseDetails.js
[error] 341-344: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (50)
src/config/appConfig.js (1)
15-15: LGTM: New author public key added.You've added a new public key to the
authorPubkeysarray, which will allow an additional author to publish content in the application.package.json (1)
41-41: Good job adding dependencies to support the Markdown rendering.These dependencies align well with your refactoring goals:
github-markdown-csswill provide consistent styling for the markdown contentremark-breakswill handle line breaks properly in the markdownBoth of these support the new
MarkdownDisplaycomponent that's replacing the dynamicMDDisplayimports.Also applies to: 54-54
src/components/content/courses/DraftCourseLesson.js (1)
9-9: Successfully refactored to use the new MarkdownDisplay component.You've replaced the dynamic import of
MDDisplaywith a static import ofMarkdownDisplayand updated the prop fromsourcetocontent. This change is consistent with the overall refactoring approach in this PR and brings several benefits:
- Improved performance by using static imports
- Better markdown rendering with GitHub styling
- Proper handling of line breaks through the
remarkBreaksplugin- Custom rendering for links and images
Also applies to: 148-148
src/components/content/combined/CombinedDetails.js (1)
17-17: Successfully refactored both Markdown rendering instances.You've consistently implemented the
MarkdownDisplaycomponent in both scenarios:
- For rendering decrypted content (paid content)
- For rendering regular content
The prop change from
sourcetocontentis appropriate, and you've maintained different styling for each use case with appropriate class names. This change aligns well with the overall refactoring approach in the PR.Also applies to: 204-204, 232-232
src/components/content/documents/DocumentDetails.js (3)
17-17: Appropriate import replacement for Markdown rendering.The static import of
MarkdownDisplayreplaces the previous dynamic import ofMDDisplay. This is part of a broader refactoring to standardize Markdown rendering across the application.
210-210: Prop name correctly updated to match new component requirements.The
sourceprop has been replaced withcontentto match the API of the newMarkdownDisplaycomponent. This aligns with the component's expected props as defined insrc/components/markdown/MarkdownDisplay.js.
236-236: Consistent prop naming in second usage.This second instance of
MarkdownDisplayalso correctly uses thecontentprop instead ofsource, maintaining consistency throughout the application.src/components/content/courses/DraftCourseDetails.js (2)
20-20: Static import replaces dynamic import for Markdown rendering.The import of
MarkdownDisplayaligns with the application-wide refactoring of Markdown display components.
466-467: Prop name updated to follow new component requirements.The
contentprop is now correctly used instead ofsourcewhen passing the Markdown content to theMarkdownDisplaycomponent.src/components/content/courses/DocumentLesson.js (3)
16-16: Appropriate import for standardized Markdown rendering.The static import of
MarkdownDisplayaligns with the application-wide refactoring of Markdown display components.
117-117: Prop name updated for paid decrypted content rendering.The
contentprop is correctly used for the first use case - rendering paid content that has been decrypted.
132-132: Prop name updated for regular content rendering.The second instance also correctly uses the
contentprop for rendering the lesson content when available.src/components/menutab/MenuTab.js (4)
3-4: Added necessary imports for sidebar toggle functionality.The imports for
ButtonandGenericButtonare needed for the new sidebar toggle feature being added to this component.
6-6: Component signature updated to support sidebar functionality.The function signature has been extended to accept three new parameters:
sidebarVisible,onToggleSidebar, andisMobileViewwith a sensible default. This aligns with the updated requirements for the component as seen in the course pages.
8-8: Added positioning context for toggle button.The
relativeclass added to the wrapper div is necessary for the absolute positioning of the sidebar toggle button.
25-42: Added conditional sidebar toggle button with appropriate behavior.The sidebar toggle button implementation:
- Is conditionally rendered only in non-mobile views
- Changes its icon based on the sidebar visibility state
- Includes accessibility features (tooltip and aria-label)
- Is properly positioned at the right side of the component
This enhances the user experience by providing an intuitive way to toggle the course lessons sidebar.
src/components/markdown/MarkdownDisplay.js (1)
1-27: Good implementation of the Markdown renderer component!The component is well-structured and follows best practices: checks for empty content, applies proper styling with GitHub markdown theme, and implements security measures for external links. The customization of link and image components is a nice touch.
src/components/content/courses/CourseHeader.js (1)
57-80: LGTM: Well-structured course header with good navigation logicThe implementation of the standard mode header is clean, with good use of positioning, responsive design, and navigation handling.
src/components/content/courses/CombinedLesson.js (3)
17-17: LGTM: Good replacement of dynamic import with static importThis change simplifies the component usage and potentially improves performance by using static imports.
174-174: Successfully updated component and prop nameThe component has been properly updated from MDDisplay to MarkdownDisplay, with the prop name changed from
sourcetocontent.
216-216: Successfully updated component and prop nameThe component has been properly updated from MDDisplay to MarkdownDisplay, with the prop name changed from
sourcetocontent.src/components/content/courses/CourseLesson.js (3)
15-15: LGTM: Good replacement of dynamic import with static importThis change simplifies the component usage and potentially improves performance by using static imports.
113-113: Successfully updated component and prop nameThe component has been properly updated from MDDisplay to MarkdownDisplay, with the prop name changed from
sourcetocontent.
123-123: Successfully updated component and prop nameThe component has been properly updated from MDDisplay to MarkdownDisplay, with the prop name changed from
sourcetocontent.src/components/content/videos/VideoDetails.js (3)
17-17: Clean import replacement with MarkdownDisplay component.The change from dynamic import of MDDisplay to a static import of MarkdownDisplay is a good improvement that should simplify the code and potentially improve load performance.
207-207: Properly updated component usage with correct props.The component has been correctly updated to use the
contentprop instead ofsourcewhile maintaining the same className for consistent styling.
240-240: Properly updated component usage with correct props.Same as above, the component has been correctly updated to use the
contentprop while maintaining styling consistency.src/components/content/courses/VideoLesson.js (3)
16-16: Clean import replacement with MarkdownDisplay component.The change from dynamic import of MDDisplay to a static import of MarkdownDisplay is a good improvement that should simplify the code and potentially improve load performance.
169-169: Properly updated component usage with correct props.The component has been correctly updated to use the
contentprop instead ofsourcewhile maintaining the same className for consistent styling and preserving the ref usage for tracking video elements.
195-195: Properly updated component usage with correct props.Same as above, the component has been correctly updated to use the
contentprop while maintaining styling consistency and preserving the ref for tracking functionality.src/pages/draft/[slug]/index.js (2)
22-22: Clean import replacement with MarkdownDisplay component.The change from dynamic import of MDDisplay to a static import of MarkdownDisplay is a good improvement that should simplify the code and potentially improve load performance.
432-432: Properly updated component usage with correct props.The component has been correctly updated to use the
contentprop instead ofsourcewhile maintaining the conditional rendering check for content availability.src/components/content/courses/CourseSidebar.js (2)
1-155: Well-structured new component for course navigation.This new CourseSidebar component is well-designed with good separation of concerns, handling both mobile and desktop layouts appropriately. The state management with parent component synchronization is implemented effectively.
50-80: Well-implemented lesson item with responsive design.The LessonItem component efficiently handles both mobile and desktop views with appropriate conditional styling. The use of Image component with proper alt text and the completed tag display is well implemented.
src/components/navbar/Navbar.js (2)
11-14: New imports are appropriate.
These imports are utilized correctly in the subsequent code (e.g.,useNDKContext,nip19.decode,parseCourseEvent,CourseHeader). No issues identified.
24-24: Use of NDK context.
Acquiringndkfrom the context is a well-structured approach; it cleanly centralizes networking/fetch capabilities.src/components/content/courses/CourseDetails.js (2)
24-24: New Divider import.
This import is used effectively later for clarity in the UI layout.
33-34: Expanded component props.
AllowingisMobileViewandshowCompletedTagto be passed in increases this component’s versatility. Good addition to support multiple device layouts and toggling completion labels.src/pages/course/[slug]/index.js (12)
1-21: Additional imports and dependencies.
Pulling inuseMemo,useWindowWidth,MenuTab,MarkdownDisplay, and others indicates a well-structured approach to unify your new tabbed UI and markdown rendering. No issues here.
174-181: State for lesson navigation and layout.
activeIndex,sidebarVisible,nAddress,isMobileView, andactiveTabare nicely separated. This should keep the code organized for multi-tab logic and responsive design.
194-210: Automated tab selection for lesson routes.
Switching to'content'tab whenactiveis specified is a great user experience. The logic for togglingsidebarVisiblebased on mobile vs. desktop is also clear.
248-254: Memoizing course completion.
UsinguseMemoto check if any lesson is completed is an efficient approach for derived state. If the completion logic grows (e.g., all lessons must be completed), you can adapt the check easily.
256-265: handleLessonSelect sets route and tab.
This ensures direct linking to a lesson while seamlessly switching tabs on mobile. Nicely done.
284-302: Tab switching logic.
Mapping tab indices to labels is a clean approach. Special-casing'lessons'for mobile is well-handled.
304-306: Sidebar toggle.
Straightforward state toggle for desktop usage.
308-316: getActiveTabIndex for bridging name→index.
KeepsMenuTabin sync with your custom tab logic. Good design.
318-346: Dynamically building tab items.
Injecting the “Lessons” tab only on mobile is a nice example of adaptive UI.
348-371: renderQASection with ZapThreadsWrapper.
This is a good modular approach for embedding Nostr-based comments. The fallback textLoading comments...is helpful to the user.
373-412: renderOverviewSection with .
Leverages the newly introducedisMobileViewprop to show a tailored layout. The approach is consistent with the rest of the code.
459-575: Tab-based course layout and sidebar.
The entire restructured layout is coherent. Toggling the sidebar for desktop vs. mobile is a nice user experience touch. The final code is somewhat large but well segmented in logical blocks (overview, content, lessons, QA), making it fairly maintainable.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
src/components/content/courses/CourseSidebar.js (4)
1-21: Extract shared navbar height to maintain consistency
IfnavbarHeightis frequently used across multiple components, consider centralizing it in a shared config or constants file to avoid discrepancies.
37-82: Refine alt text for lesson images
Using"Lesson ${index + 1} thumbnail"is acceptable, but consider a more descriptive alt text, for instance includinglesson.title, to improve accessibility.
110-124: Avoid repetition with shared components
“Course Lessons” heading and lesson listing logic is also present inSidebarContent. Extracting them into a reusable subcomponent could help maintain DRY principles.
126-151: Verify sticky offset
You are using a fixed numerical offset (navbarHeight + 70). Ensure it stays aligned with any global layout changes to prevent overlap or visual misalignment.src/components/content/courses/DesktopCourseDetails.js (1)
94-112: Optional chain recommendation
To simplify description rendering, consider using an optional chain instead of&&, as static analysis suggests.Apply this diff at lines 102-105:
- {processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="mb-2">{line}</p>)} + {processedEvent.description?.split('\n') + .map((line, index) => <p key={index} className="mb-2">{line}</p>)}🧰 Tools
🪛 Biome (1.9.4)
[error] 102-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/content/courses/MobileCourseDetails.js (3)
51-57: Improve image accessibility.The alt text "avatar image" is not descriptive enough. For better accessibility, describe whose avatar it is.
<Image - alt="avatar image" + alt={`Avatar of ${author?.username || author?.name || 'course creator'}`} src={returnImageProxy(author?.avatar, author?.pubkey)} width={32} height={32} className="rounded-full mr-2" />
85-88: Use optional chaining for description rendering.As suggested by the static analysis, use optional chaining to make the code more robust against undefined values.
- {processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)} + {processedEvent.description?.split('\n') + .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)}🧰 Tools
🪛 Biome (1.9.4)
[error] 85-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
9-22: Consider adding prop validation.This component receives many props, but lacks prop validation. Consider adding PropTypes or TypeScript typing to ensure the correct props are passed.
You could add prop validation using PropTypes:
import PropTypes from 'prop-types'; // After the component definition MobileCourseDetails.propTypes = { processedEvent: PropTypes.object.isRequired, paidCourse: PropTypes.bool, lessons: PropTypes.array.isRequired, decryptionPerformed: PropTypes.bool, author: PropTypes.object, zapAmount: PropTypes.number, zapsLoading: PropTypes.bool, menuItems: PropTypes.array, returnImageProxy: PropTypes.func.isRequired, renderPaymentMessage: PropTypes.func.isRequired, isCompleted: PropTypes.bool, showCompletedTag: PropTypes.bool };src/pages/course/[slug]/index.js (5)
319-345: Memoize tab items to prevent unnecessary recalculations.The
getTabItemsfunction creates a new array on each render. Consider usinguseMemoto optimize performance, especially since this is used in the render cycle.- // Create tab items for MenuTab - const getTabItems = () => { + // Memoize tab items + const tabItems = useMemo(() => { const items = [ { label: 'Overview', icon: 'pi pi-home', }, { label: 'Content', icon: 'pi pi-book', } ]; // Add lessons tab only on mobile if (isMobileView) { items.push({ label: 'Lessons', icon: 'pi pi-list', }); } items.push({ label: 'Comments', icon: 'pi pi-comments', }); return items; - }; + }, [isMobileView]);Then update all references from
getTabItems()to justtabItems.
347-371: Memoize the QA section to prevent unnecessary re-renders.The
renderQASectionfunction creates a new component tree on each render. Consider memoizing it for better performance since it depends on only a few props.- // Render the QA section (empty for now) - const renderQASection = () => { + // Memoize the QA section + const qaSection = useMemo(() => { return ( <div className="rounded-lg p-8 mt-4 bg-gray-800"> <h2 className="text-xl font-bold mb-4">Comments</h2> {nAddress !== null ? ( <ZapThreadsWrapper anchor={nAddress} user={session?.user?.pubkey ? nip19.npubEncode(session?.user?.pubkey) : null} relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" disable="zaps" /> ) : course?.d ? ( <ZapThreadsWrapper anchor={course.d} user={session?.user?.pubkey ? nip19.npubEncode(session?.user?.pubkey) : null} relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" disable="zaps" /> ) : ( <p>Loading comments...</p> )} </div> ); - }; + }, [nAddress, session?.user?.pubkey, course?.d]);Then replace
renderQASection()calls withqaSection.
479-480: Remove unnecessary styles and improve clarity.The conditional styling approach is unnecessarily complex. The inline style is applied only when both conditions are true, but the className has redundant width values.
- <div className={`transition-all duration-500 ease-in-out ${isMobileView ? 'w-full' : 'w-full'}`} - style={!isMobileView && sidebarVisible ? {paddingRight: '320px'} : {}}> + <div className="transition-all duration-500 ease-in-out w-full" + style={!isMobileView && sidebarVisible ? {paddingRight: '320px'} : {}}>
540-543: Simplify conditional class expression.The conditional class expression is overly complex and could be simplified for better readability.
- <div className={`flex-shrink-0 transition-all duration-300 z-[999] ${ - (isMobileView && activeTab === 'lessons') ? 'ml-0 w-auto opacity-100' : - 'w-0 ml-0 opacity-0 overflow-hidden' - }`}> + <div className={`flex-shrink-0 transition-all duration-300 z-[999] ${ + (isMobileView && activeTab === 'lessons') ? 'w-auto opacity-100' : + 'w-0 opacity-0 overflow-hidden' + }`}>
189-190: Implement missing functionality for non-naddr slugs.There's a TODO comment about handling non-naddr slugs, but no implementation. This could lead to broken functionality if the course is accessed using a regular slug.
if (slug.includes('naddr')) { setNAddress(slug); } else { - // todo: no naddress? + // Create nAddress from course data when available + if (course?.pubkey && course?.kind && course?.d) { + const naddr = nip19.naddrEncode({ + pubkey: course.pubkey, + kind: course.kind, + identifier: course.d, + relays: ["wss://relay.nostr.band/", "wss://nos.lol/"] + }); + setNAddress(naddr); + } }Would you like me to implement this functionality completely or open an issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/content/courses/CourseDetails.js(4 hunks)src/components/content/courses/CourseSidebar.js(1 hunks)src/components/content/courses/DesktopCourseDetails.js(1 hunks)src/components/content/courses/MobileCourseDetails.js(1 hunks)src/pages/course/[slug]/index.js(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/components/content/courses/CourseSidebar.js (5)
src/components/content/courses/CourseDetails.js (1)
useImageProxy(39-39)src/pages/course/[slug]/index.js (5)
navbarHeight(181-181)activeIndex(174-174)isMobileView(179-179)completedLessons(175-175)lessons(91-91)src/components/navbar/Navbar.js (1)
navbarHeight(19-19)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)
src/components/content/courses/CourseDetails.js (2)
src/components/content/courses/MobileCourseDetails.js (1)
MobileCourseDetails(9-134)src/components/content/courses/DesktopCourseDetails.js (1)
DesktopCourseDetails(9-151)
src/components/content/courses/DesktopCourseDetails.js (7)
src/components/content/courses/MobileCourseDetails.js (1)
router(23-23)src/components/content/courses/CourseDetails.js (5)
router(38-38)zapAmount(35-35)menuItems(62-80)author(36-36)renderPaymentMessage(126-193)src/pages/course/[slug]/index.js (3)
router(170-170)lessons(91-91)paidCourse(26-26)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/zaps/ZapDisplay.js (1)
ZapDisplay(7-46)src/hooks/nostrQueries/zaps/useZapsSubscription.js (1)
zapsLoading(7-7)src/components/ui/MoreOptionsMenu.js (1)
MoreOptionsMenu(14-67)
src/pages/course/[slug]/index.js (5)
src/components/content/courses/CourseDetails.js (6)
nAddress(37-37)windowWidth(43-43)router(38-38)fetchAuthor(88-99)session(41-41)CourseDetails(25-233)src/hooks/useWindowWidth.js (1)
useWindowWidth(3-24)src/components/content/courses/CourseSidebar.js (2)
navbarHeight(20-20)CourseSidebar(7-152)src/components/ZapThreadsWrapper.js (1)
ZapThreadsWrapper(3-56)src/components/menutab/MenuTab.js (1)
MenuTab(6-45)
🪛 Biome (1.9.4)
src/components/content/courses/DesktopCourseDetails.js
[error] 102-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/content/courses/MobileCourseDetails.js
[error] 85-88: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (12)
src/components/content/courses/CourseSidebar.js (2)
22-35: Prop-driven visibility sync looks good
The effect ensures the sidebar’s localvisiblestate is updated wheneverparentSidebarVisiblechanges. Code appears logical and robust.
84-108: Structured sidebar content
The header and close icon handling are straightforward and easy to maintain. Good separation of concerns here.src/components/content/courses/CourseDetails.js (5)
21-23: New mobile/desktop imports
Introducing bothDesktopCourseDetailsandMobileCourseDetailskeeps the layout code clean and well-organized.
32-33: Enhanced configuration
AddingisMobileViewandshowCompletedTagbroadens flexibility for rendering conditional states. Implementation looks solid.
44-45: Phone detection fallback
CombiningisMobileViewwith a local window-width check ensures comprehensive mobile detection. Nicely done.
203-217: Centralized props for clarity
Collecting shared props indetailsPropsreduces repetition and improves readability across desktop and mobile components.
220-229: Conditional rendering flow
Switching between mobile and desktop details withisPhoneis a clean approach. No issues spotted in the final return statement.src/components/content/courses/DesktopCourseDetails.js (5)
1-8: Initial imports and setup
Imports are correct, referencingnext/imageand other required dependencies without redundancy.
9-22: Well-defined prop signature
All props—course data, rendering callbacks, and flags—are encapsulated neatly. Solid structure for the component’s needs.
25-60: Header layout
Displays course image, completion status, and user options effectively. The structure is appropriately modular and user-friendly.
62-90: Topic tags and author details
Proper usage of tags; the avatar is fetched viareturnImageProxy. No issues found with data rendering here.
114-148: Detailed course info
Displays lesson count, pricing, and publication date cleanly. Implementation aligns with best practices for content layout.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/course/[slug]/index.js (1)
367-392: 🛠️ Refactor suggestionExtract relay URLs to a constant.
The relay URLs are duplicated in the
renderQASectionfunction at lines 376 and 383.+ // Add at the top of the component or in a shared config file + const NOSTR_RELAYS = "wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/"; // Then in renderQASection: <ZapThreadsWrapper anchor={nAddress} user={session?.user?.pubkey ? nip19.npubEncode(session?.user?.pubkey) : null} - relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" + relays={NOSTR_RELAYS} disable="zaps" /> // And in the second instance: <ZapThreadsWrapper anchor={course.d} user={session?.user?.pubkey ? nip19.npubEncode(session?.user?.pubkey) : null} - relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" + relays={NOSTR_RELAYS} disable="zaps" />
🧹 Nitpick comments (2)
src/components/content/courses/MobileCourseDetails.js (2)
30-36: Implement optional chaining for topics display.The component handles the display of course topics correctly but could benefit from using optional chaining.
- {processedEvent.topics && - processedEvent.topics.length > 0 && - processedEvent.topics.map((topic, index) => ( + {processedEvent.topics?.length > 0 && + processedEvent.topics.map((topic, index) => (
83-96: Course description section with proper text handling.The component displays the course description with proper formatting by splitting text into paragraphs.
Based on the static analysis hint, consider using optional chaining for the description to prevent potential errors:
- {processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)} + {processedEvent.description?.split('\n') + .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)}🧰 Tools
🪛 Biome (1.9.4)
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/content/courses/MobileCourseDetails.js(1 hunks)src/config/appConfig.js(1 hunks)src/pages/course/[slug]/index.js(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/config/appConfig.js
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/pages/course/[slug]/index.js (3)
src/components/content/courses/CourseSidebar.js (2)
navbarHeight(20-20)CourseSidebar(7-152)src/components/menutab/MenuTab.js (1)
MenuTab(6-45)src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/MobileCourseDetails.js (7)
src/components/content/courses/DesktopCourseDetails.js (1)
router(23-23)src/pages/course/[slug]/index.js (3)
router(170-170)lessons(91-91)paidCourse(26-26)src/components/content/courses/CourseDetails.js (5)
router(38-38)zapAmount(35-35)author(36-36)menuItems(62-80)renderPaymentMessage(126-193)src/components/zaps/ZapDisplay.js (1)
ZapDisplay(7-46)src/hooks/nostrQueries/zaps/useZapsSubscription.js (1)
zapsLoading(7-7)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/ui/MoreOptionsMenu.js (1)
MoreOptionsMenu(14-67)
🪛 Biome (1.9.4)
src/components/content/courses/MobileCourseDetails.js
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (17)
src/components/content/courses/MobileCourseDetails.js (6)
1-8: Correct imports usage for MobileCourseDetails.The component imports all necessary dependencies, including React, Next.js components, and PrimeReact UI components.
9-22: Well-structured props interface for MobileCourseDetails.The component accepts a comprehensive set of props for displaying course details, handling payment actions, and managing user interactions in a mobile context.
23-24: Router initialization for navigation.The component correctly initializes the Next.js router for handling navigation.
38-46: Effective title and zaps display.The component correctly displays the course title and zap amount in a responsive layout with proper alignment.
48-75: Author information and more options display.The author information section is well-implemented with proper avatar display, username fallback, and external link handling.
98-131: Well-structured course information section.The component displays course information in a well-organized layout, showing lesson count, price (if applicable), and publication date with appropriate conditional rendering.
src/pages/course/[slug]/index.js (11)
1-21: Updated imports for enhanced course page functionality.The component imports additional critical dependencies for the new tabbed interface and responsive layout, including
useMemo,CourseSidebar,CourseHeader,MenuTab, andMarkdownDisplay.
174-182: Updated state management for course navigation.The component replaces
expandedIndexwithactiveIndexand adds new state variables for sidebar visibility, nAddress, and tab management, along with responsive view detection.
183-210: Effective initialization logic for course navigation.The useEffect hooks correctly initialize the nAddress and handle tab/lesson selection based on URL parameters, with proper responsive behavior handling for sidebar visibility.
248-265: Improved course completion logic and lesson selection.The component uses
useMemofor checking course completion status and implements an improved lesson selection handler with router integration.
284-316: Well-structured tab navigation logic.The component implements comprehensive tab management with proper mapping between tab indices and names, handling mobile-specific logic correctly.
318-345: Dynamic tab items generation based on device type.The component intelligently generates tab items based on whether the view is mobile or desktop, adjusting the UI appropriately.
347-365: Keyboard navigation support for accessibility.The implementation adds keyboard navigation support for tabs, enhancing accessibility.
393-432: Well-implemented overview section with responsive design.The
renderOverviewSectionfunction properly handles the course overview display with conditional rendering for mobile view and completed course status.
479-494: Improved navigation with sticky tab menu.The implementation uses a sticky tab navigation system at the top of the page, positioned correctly relative to the navbar.
495-529: Responsive content layout with tab-based content switching.The main content area is well-structured with proper transitions between tabs, conditional rendering, and responsive width adjustments.
530-584: Responsive sidebar implementation with device-specific behavior.The sidebar implementation differs between desktop and mobile views, with appropriate positioning, animations, and interaction handling for each context.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/components/content/courses/VideoLesson.js (1)
195-195: Duplicate comment regarding raw HTML rendering.This
MarkdownDisplaycall shares the same XSS concern iflesson.contentis untrusted. Review sanitization measures or confirm that the content is safe.src/pages/course/[slug]/index.js (1)
367-391: Repeated relay URLs.
These relay URLs are hardcoded here and also appear in other places. This matches a previous recommendation to extract them into a constant or config.
🧹 Nitpick comments (14)
src/components/content/courses/CourseSidebar.js (3)
37-82: Use a stable key for list items.Currently, each
<LessonItem>is keyed by its index. If the lesson list changes order or is updated dynamically, it can lead to stale rendering. If possible, use a unique identifier likelesson.id.- {lessons.map((lesson, index) => ( - <LessonItem key={index} ... + {lessons.map((lesson) => ( + <LessonItem key={lesson.id} ...
85-109: Refactor repeated lesson listing into a shared component.
SidebarContentduplicates the lesson iteration logic found inMobileLessonsTab. Extracting a shared<LessonList>or<Lessons>component can reduce code duplication and simplify maintenance.
111-124: MobileLessonsTab duplicates lesson rendering.As noted above, centralizing the structure for lessons into a shared component would reduce duplication between desktop sidebar and mobile layout.
src/components/content/courses/CourseDetails.js (2)
32-33: Optional chaining for isMobileViewAccepting
isMobileViewas a prop and defaultingshowCompletedTagtotruehelps maintain flexible layouts. Make sure to document these props in the component’s docstring or usage references.
44-45: Dual mobile detection logic.You're combining
isMobileViewwithlocalIsMobileViewintoisPhone. This is fine for a short-term fix. However, consider consolidating the logic to one approach to avoid potential confusion.src/components/content/courses/DesktopCourseDetails.js (2)
82-90: Missing href on anchor tag.This anchor is displayed as a link but does not have an
hrefattribute. If navigation is intended, add anhref; otherwise, convert it to a<span>or<div>.<a rel="noreferrer noopener" target="_blank" + href={`/profile/${author?.pubkey || ''}`} className="text-blue-300 hover:underline" > {author?.username || author?.name || author?.pubkey} </a>
102-105: Consider using optional chaining.Using
processedEvent.description?.split('\n')prevents accidental runtime errors ifdescriptionis undefined or null, though the current conditional check also works.- {processedEvent.description && - processedEvent.description.split('\n').map(...)} + {processedEvent.description?.split('\n')?.map(...)}🧰 Tools
🪛 Biome (1.9.4)
[error] 102-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/content/courses/MobileCourseDetails.js (1)
86-89: Leverage optional chaining for readability.Replacing
processedEvent.description && processedEvent.description.split('\n')with optional chaining can simplify the expression.- {processedEvent.description && - processedEvent.description.split('\n').map(...)} + {processedEvent.description?.split('\n')?.map(...)}🧰 Tools
🪛 Biome (1.9.4)
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/navbar/Navbar.js (1)
117-143: Loading state consideration.Currently, the brand is displayed while the course data loads. You may consider a short loading indicator or skeleton to avoid flickering if
coursetakes time to resolve.src/pages/course/[slug]/index.js (5)
8-9: UnusedCourseHeaderimport.
CourseHeaderis imported but never referenced. If it’s no longer needed, please remove it to keep the code clean and avoid confusion.import CourseSidebar from '@/components/content/courses/CourseSidebar'; -import CourseHeader from '@/components/content/courses/CourseHeader';
183-192: StoringnAddressfrom the router.
ThisuseEffectproperly checks if the slug is an naddr and setsnAddress. Consider verifying ifslugmight be undefined or empty before callingslug.includes(...)to avoid potential errors.if (router.isReady && slug) { if (slug.includes('naddr')) { setNAddress(slug); } }
256-265:handleLessonSelectfor lesson transitions and shallow routing.
This properly updates the URL'sactiveparam and toggles the content tab in mobile view. You may consider scrolling the page to top on lesson select for improved user experience.const handleLessonSelect = (index) => { setActiveIndex(index); router.push(...); + window.scrollTo({ top: 0, behavior: 'smooth' }); ... };
318-345:getTabItemsfor building theMenuTablist.
This function properly adds “Lessons” on mobile only. If the tab structure grows more complex, consider a single source of truth for bothgetTabItemsandtoggleTab.
347-365: Keyboard navigation for tab switching.
Attaching listeners for left/right arrow keys is great for accessibility. Consider addingHomeandEndkey handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
package.json(2 hunks)src/components/content/combined/CombinedDetails.js(3 hunks)src/components/content/courses/CombinedLesson.js(3 hunks)src/components/content/courses/CourseDetails.js(4 hunks)src/components/content/courses/CourseHeader.js(1 hunks)src/components/content/courses/CourseLesson.js(3 hunks)src/components/content/courses/CourseSidebar.js(1 hunks)src/components/content/courses/DesktopCourseDetails.js(1 hunks)src/components/content/courses/DocumentLesson.js(3 hunks)src/components/content/courses/DraftCourseDetails.js(2 hunks)src/components/content/courses/DraftCourseLesson.js(2 hunks)src/components/content/courses/MobileCourseDetails.js(1 hunks)src/components/content/courses/VideoLesson.js(3 hunks)src/components/content/documents/DocumentDetails.js(3 hunks)src/components/content/videos/VideoDetails.js(3 hunks)src/components/markdown/MarkdownDisplay.js(1 hunks)src/components/menutab/MenuTab.js(2 hunks)src/components/navbar/Navbar.js(3 hunks)src/components/profile/DataTables/UserProgressTable.js(0 hunks)src/config/appConfig.js(1 hunks)src/pages/course/[slug]/index.js(7 hunks)src/pages/draft/[slug]/index.js(2 hunks)
💤 Files with no reviewable changes (1)
- src/components/profile/DataTables/UserProgressTable.js
🧰 Additional context used
🧬 Code Graph Analysis (17)
src/components/content/courses/DraftCourseDetails.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/DocumentLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/CourseDetails.js (2)
src/components/content/courses/MobileCourseDetails.js (1)
MobileCourseDetails(9-135)src/components/content/courses/DesktopCourseDetails.js (1)
DesktopCourseDetails(9-151)
src/components/content/courses/DraftCourseLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/videos/VideoDetails.js (2)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)src/pages/details/[slug]/index.js (1)
decryptedContent(22-22)
src/pages/draft/[slug]/index.js (3)
src/pages/draft/[slug]/edit.js (1)
draft(11-11)src/pages/course/[slug]/draft/edit.js (1)
draft(10-10)src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/combined/CombinedDetails.js (2)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)src/pages/details/[slug]/index.js (1)
decryptedContent(22-22)
src/components/menutab/MenuTab.js (2)
src/pages/course/[slug]/index.js (2)
isMobileView(179-179)sidebarVisible(176-176)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)
src/components/content/courses/CourseSidebar.js (5)
src/components/content/courses/CourseDetails.js (1)
useImageProxy(39-39)src/pages/course/[slug]/index.js (5)
navbarHeight(181-181)activeIndex(174-174)isMobileView(179-179)completedLessons(175-175)lessons(91-91)src/components/navbar/Navbar.js (1)
navbarHeight(19-19)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)
src/components/content/courses/CourseLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/CourseHeader.js (5)
src/components/content/courses/CourseDetails.js (1)
router(38-38)src/pages/course/[slug]/index.js (4)
router(170-170)course(24-24)isMobileView(179-179)navbarHeight(181-181)src/components/navbar/Navbar.js (3)
router(17-17)course(25-25)navbarHeight(19-19)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)src/components/content/courses/CourseSidebar.js (1)
navbarHeight(20-20)
src/components/content/courses/CombinedLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/VideoLesson.js (1)
src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
src/components/content/courses/MobileCourseDetails.js (7)
src/components/content/courses/DesktopCourseDetails.js (1)
router(23-23)src/components/content/courses/CourseDetails.js (5)
router(38-38)zapAmount(35-35)author(36-36)menuItems(62-80)renderPaymentMessage(126-193)src/pages/course/[slug]/index.js (3)
router(170-170)lessons(91-91)paidCourse(26-26)src/components/zaps/ZapDisplay.js (1)
ZapDisplay(7-46)src/hooks/nostrQueries/zaps/useZapsSubscription.js (1)
zapsLoading(7-7)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/ui/MoreOptionsMenu.js (1)
MoreOptionsMenu(14-67)
src/components/content/courses/DesktopCourseDetails.js (7)
src/components/content/courses/MobileCourseDetails.js (1)
router(23-23)src/components/content/courses/CourseDetails.js (5)
router(38-38)zapAmount(35-35)menuItems(62-80)author(36-36)renderPaymentMessage(126-193)src/pages/course/[slug]/index.js (3)
router(170-170)lessons(91-91)paidCourse(26-26)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/zaps/ZapDisplay.js (1)
ZapDisplay(7-46)src/hooks/nostrQueries/zaps/useZapsSubscription.js (1)
zapsLoading(7-7)src/components/ui/MoreOptionsMenu.js (1)
MoreOptionsMenu(14-67)
src/components/navbar/Navbar.js (2)
src/components/content/courses/CourseHeader.js (2)
router(12-12)CourseHeader(6-80)src/utils/nostr.js (2)
parseCourseEvent(149-218)parseCourseEvent(149-218)
src/pages/course/[slug]/index.js (3)
src/components/content/courses/CourseDetails.js (6)
nAddress(37-37)windowWidth(43-43)router(38-38)fetchAuthor(88-99)session(41-41)CourseDetails(25-233)src/components/ZapThreadsWrapper.js (1)
ZapThreadsWrapper(3-56)src/components/menutab/MenuTab.js (1)
MenuTab(6-45)
🪛 Biome (1.9.4)
src/components/content/courses/MobileCourseDetails.js
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/content/courses/DesktopCourseDetails.js
[error] 102-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (63)
src/config/appConfig.js (1)
14-14: Added new author public key to authorPubkeys array.The addition of the new public key appears to be correct and properly formatted. This change extends the list of authorized authors in the application.
package.json (2)
41-41: Added github-markdown-css dependency.Good addition. This package will provide GitHub-style formatting for rendered markdown content, which aligns with the overall refactoring of markdown display components in this PR.
54-54: Added remark-breaks dependency.This plugin enables proper handling of line breaks in markdown content. It works well with the new MarkdownDisplay component to ensure consistent rendering across the application.
src/components/content/courses/DraftCourseLesson.js (2)
9-9: Replaced dynamic import with static import of MarkdownDisplay.This change is part of the broader refactoring to standardize markdown rendering across the application. The static import is more efficient than the previous dynamic import.
148-148: Updated markdown component and prop name.Successfully migrated from MDDisplay with
sourceprop to MarkdownDisplay withcontentprop while maintaining the same styling. This change aligns with the application-wide standardization of markdown rendering.src/components/content/courses/DraftCourseDetails.js (2)
20-20: Replaced dynamic import with static import of MarkdownDisplay.Good change. Using a static import instead of a dynamic one simplifies the code and improves maintainability.
466-466: Updated markdown component and prop name.Successfully replaced MDDisplay with MarkdownDisplay and updated the prop name from
sourcetocontent. This maintains consistency with other similar changes throughout the application.src/components/content/documents/DocumentDetails.js (3)
17-17: Import refactored to use static component import.The change from dynamically importing
MDDisplayto statically importingMarkdownDisplaywill improve the initial load performance and code organization.
210-210: Prop name updated for consistency across the application.The component has been updated from
MDDisplaywithsourceprop toMarkdownDisplaywithcontentprop, which follows the naming convention of the new component as defined in theMarkdownDisplay.jsfile.
236-236: Component and prop updated to use new markdown renderer.Same pattern as above - updated to use the standardized
MarkdownDisplaycomponent with thecontentprop.src/components/content/combined/CombinedDetails.js (3)
17-17: Import refactored to use static component import.The change from dynamically importing
MDDisplayto statically importingMarkdownDisplayimproves code maintainability and load performance.
204-204: Prop name updated for consistency across the application.The component has been updated from
MDDisplaywithsourceprop toMarkdownDisplaywithcontentprop, which follows the API defined in the new component.
232-232: Component and prop updated to use new markdown renderer.Consistent with the pattern applied throughout the codebase - updated to use the standardized
MarkdownDisplaycomponent with thecontentprop.src/components/content/courses/DocumentLesson.js (3)
16-16: Import refactored to use static component import.The change from dynamically importing
MDDisplayto statically importingMarkdownDisplayimproves code organization and bundle management.
117-117: Prop name updated for consistency across the application.The component has been updated from
MDDisplaywithsourceprop toMarkdownDisplaywithcontentprop, maintaining consistency with the new markdown rendering implementation.
132-132: Component and prop updated to use new markdown renderer.Consistent implementation of the new markdown rendering approach using
MarkdownDisplaywith thecontentprop.src/components/content/videos/VideoDetails.js (3)
17-17: Import refactored to use static component import.The change from dynamically importing
MDDisplayto statically importingMarkdownDisplaysimplifies the code and improves load performance.
207-207: Prop name updated for consistency across the application.The component has been updated from
MDDisplaywithsourceprop toMarkdownDisplaywithcontentprop, which aligns with the naming convention used in the new component.
240-240: Component and prop updated to use new markdown renderer.Continuing the pattern applied throughout the codebase - updated to use the standardized
MarkdownDisplaycomponent with thecontentprop.src/components/menutab/MenuTab.js (4)
3-4: Import statements properly added for the sidebar toggle button.The Button import from PrimeReact and GenericButton from the local components directory are correctly added to support the new sidebar toggle functionality.
6-6: Well-structured function signature update.The component now accepts three additional props (
sidebarVisible,onToggleSidebar, andisMobileView) with a sensible default value forisMobileView. This enables the sidebar toggle functionality and responsive behavior.
8-8: Appropriate positioning class added.Adding the
relativeclass to the wrapper div is necessary to properly position the absolute-positioned sidebar toggle button that's being added.
25-42: Clean implementation of the sidebar toggle button.The sidebar toggle button implementation:
- Is correctly conditional based on viewport size
- Uses appropriate positioning
- Has dynamic icon and tooltip based on sidebar state
- Includes proper accessibility attributes
- Maintains clean separation of concerns
The code is well-structured and follows good UI practices.
src/components/markdown/MarkdownDisplay.js (3)
1-7: Proper imports for Markdown rendering.The component imports all necessary dependencies for enhanced Markdown rendering:
- ReactMarkdown for core functionality
- rehypeRaw to support raw HTML
- remarkGfm for GitHub Flavored Markdown
- remarkBreaks for better line break handling
- GitHub markdown CSS for consistent styling
This provides a comprehensive solution for Markdown rendering.
8-25: Well-implemented Markdown renderer with safety features.This implementation:
- Handles null content gracefully
- Uses appropriate plugins for enhanced markdown features
- Makes external links secure with
target="_blank"andrel="noopener noreferrer"- Applies consistent image styling
- Properly handles className extension for customization
The component effectively centralizes Markdown rendering logic that was previously scattered across the application.
27-27: Export implemented correctly.The component is properly exported as the default export.
src/components/content/courses/CourseHeader.js (4)
1-11: Well-structured component declaration with appropriate imports.The component imports necessary dependencies and defines a clean function signature with sensible default values. The
isNavbarModeprop with defaultfalseallows for versatile rendering in different contexts.
14-25: Smart navigation handling with context awareness.The
handleBackNavigationfunction intelligently determines where to navigate based on the current context:
- If viewing a specific lesson, it returns to the course overview
- If viewing the course overview, it returns to the home page
This creates an intuitive user experience with context-aware back navigation.
27-55: Clean implementation of navbar mode with event handling.The navbar mode implementation:
- Conditionally renders based on the
isNavbarModeprop- Has proper event propagation control in the nested button
- Uses appropriate styling for compact display
- Implements truncation for long course names
- Has responsive text sizing based on viewport
The component effectively serves as a compact course header for the navbar.
58-79: Well-implemented standard mode with proper positioning.The standard mode implementation:
- Uses sticky positioning with appropriate z-index
- Properly offsets from the navbar using the provided height
- Implements appropriate styling and spacing
- Has responsive text sizing
- Includes accessible controls with tooltips
The component provides a consistent header experience for course content.
src/components/content/courses/CombinedLesson.js (3)
17-17: Import updated from dynamic to static.The component now directly imports
MarkdownDisplayinstead of using dynamic import forMDDisplay. This simplifies the code and aligns with the broader refactoring effort across the application.
174-174: Component and prop names updated consistently.The
MDDisplaycomponent has been replaced withMarkdownDisplayand the prop name has been updated fromsourcetocontentwhile maintaining the same className for styling.
216-216: Component and prop names updated consistently.The same component and prop name updates have been applied consistently in this second instance, ensuring a uniform implementation throughout the file.
src/components/content/courses/CourseLesson.js (1)
15-15: Direct import of MarkdownDisplay.Switching from a dynamic import to a named import streamlines the rendering of Markdown content and may simplify the code. However, be mindful that this removes any potential code-splitting benefits that dynamic imports might have offered.
src/pages/draft/[slug]/index.js (1)
22-22: Importing MarkdownDisplay directly.Good job replacing dynamic imports with a direct import for a cleaner approach. Keep in mind this can slightly increase initial load but provides simpler maintainability.
src/components/content/courses/CourseSidebar.js (1)
126-155:Details
✅ Verification successful
Z-index and styling considerations.
The sidebar uses
z-[999]which can conflict with other layered UI elements (e.g., modals or tooltips). Verify that 999 is an appropriate stacking context for your layout. Also, watch for consistent application of top offsets (navbarHeight + 70) to avoid layout shifts.
🏁 Script executed:
#!/bin/bash # Searching the codebase for other z-index values that might conflict rg -A 2 'z-\['Length of output: 635
Stacking Context Consistency and Offset Confirmation
- The use of
z-[999]in bothCourseSidebar.jsandsrc/pages/course/[slug]/index.jsshows that the stacking context was intentionally unified across these components.- Please ensure that this z-index value is sufficient to handle any higher-layer elements (e.g., modals or tooltips) that might appear in the future.
- Also, verify that the top offset calculation (
navbarHeight + 70px) remains consistent with header spacing changes to prevent layout shifts.src/components/content/courses/CourseDetails.js (3)
20-23: Clear labeling for imported components.Great job introducing a clear comment block (
// Import the desktop and mobile components). This improves readability and makes it evident which layout components are being used.
203-218: Good DRY approach with shared props.Creating a
detailsPropsobject centralizes the data passed to both desktop and mobile views. This is a neat approach to reduce duplication between the two layouts.
220-229: Conditional rendering of layout components.Pulling the large render logic into separate
MobileCourseDetailsandDesktopCourseDetailscomponents increases maintainability. This approach also seems to align well with yourisPhonecheck.src/components/content/courses/VideoLesson.js (2)
16-16: Confirm SSR compatibility when removing dynamic import.Previously, the
VideoLessoncomponent useddynamic(() => import(...), { ssr: false }). Switching to a direct import may cause SSR issues ifMarkdownDisplayreferences the browser environment. Please verify it is safe for SSR in all usage contexts.
169-169: Ensure content is properly sanitized.
rehypeRawprocesses raw HTML, which can open the door to XSS iflesson.contentis untrusted. Consider using a sanitizing library or removingrehypeRawunless you intentionally want to render arbitrary HTML.src/components/content/courses/DesktopCourseDetails.js (1)
1-22: File-level overview check.This entire file is newly added. No syntax or critical logic issues are observed in imports and declarations. Looks good overall!
src/components/content/courses/MobileCourseDetails.js (1)
1-8: File-level overview check.This mobile-specific component structure appears consistent with the desktop version. No obvious logic or performance concerns.
src/components/navbar/Navbar.js (3)
11-14: New imports appear valid.Imports for
CourseHeader,useNDKContext,nip19, andparseCourseEventlook correct and are used consistently.
24-30: Track course state in the navbar.Storing
courseandisCompletedhere helps provide a contextual navbar. Check if the approach is consistent with the overall app architecture.
32-68: Evaluate multiple event results & array type.When calling
ndk.fetchEvent({ '#d': [identifier] }), ensure it’s guaranteed to return a single event. Also confirmcompletedCoursesis always an array to avoid unexpected issues withincludes().src/pages/course/[slug]/index.js (17)
1-1: No issues with the added React hooks import.
You're now importinguseMemo, which is used later forisCourseCompleted. This looks correct.
12-12: Addednip19import is appropriate.
Importingnip19for decoding and encoding addresses is valid. Looks good.
17-17:ZapThreadsWrapperimport.
This import is consistent with its usage in the QA/comments section. No issues.
18-21: New hooks and components imports.
These imports (useWindowWidth,MenuTab,Tag,MarkdownDisplay) align with usage for responsive layouts, tab navigation, tagging, and Markdown rendering. Everything seems consistent.
174-181: New state variables introduced for navigation and UI control.
DefiningactiveIndex,sidebarVisible,nAddress,isMobileView, andactiveTabhelps manage tabs and layout responsiveness. All look fine. Just ensure thatCourseHeaderis truly unused before keeping that import.
194-210: Synchronizing active lesson and sidebar behavior.
ThisuseEffectsets the active lesson from the URL query and adjusts the tab/view accordingly. Looks correct. Consider falling back to0ifparseIntfails. Currently, ifactiveis invalid, it could becomeNaN.const parsedIndex = parseInt(active, 10); if (!isNaN(parsedIndex)) { setActiveIndex(parsedIndex); setActiveTab('content'); } else { setActiveIndex(0); setActiveTab('overview'); }
232-232: Empty line insertion.
This spacing change has no functional effect.
239-239: Empty line insertion.
No impact on functionality.
248-254:isCourseCompletedlogic.
You're usinguseMemoto determine course completion if at least one lesson is completed. Ensure this meets the desired definition of “course completion.” This is a valid approach if partial completion is acceptable.
284-302: Tab switching logic with dynamic insertion of “lessons” for mobile.
Conditionally adding "lessons" in the middle of thetabMapis a neat approach. Just note that if more tabs are added later, the insertion index might require maintenance.
304-306: SimplehandleToggleSidebar.
Toggles the boolean correctly. Straightforward and functional.
392-431:renderOverviewSection
Neatly encapsulates the display for mobile vs. desktop. The usage of<CourseDetails>withshowCompletedTagensures consistent styling. Looks good.
452-452: Minor change at line 452.
No functional changes. Possibly just formatting.
478-494: Sticky tab navigation UI.
Ensuring theMenuTabis pinned below thenavbarHeightwith a border for visual delineation is a nice detail. The approach is clear and straightforward.
496-529: Conditional display of overview, content, and QA.
The usage ofblock/hiddenclasses for tab switching is fine. Alternatively, you could map each tab to a small component for clarity, but this is acceptable.
531-559: Desktop sidebar toggling.
Positioning the sidebar absolutely on desktop devices and adjusting translation is performed correctly. Good job.
560-584: Mobile sidebar toggling and lesson selection.
Dynamic switching between tabs and toggling the sidebar ensures a decent mobile experience. No major issues identified.
| } | ||
| if (lesson?.content) { | ||
| return <MDDisplay className="p-4 rounded-lg w-full" source={lesson.content} />; | ||
| return <MarkdownDisplay content={lesson.content} className="p-4 rounded-lg w-full" />; |
There was a problem hiding this comment.
Repeated raw HTML rendering concern.
As with the previous usage, ensure lesson.content is sanitized or from a trusted source. The same recommendation applies here to mitigate XSS risks.
| const renderContent = () => { | ||
| if (isPaid && decryptionPerformed) { | ||
| return <MDDisplay className="p-4 rounded-lg w-full" source={lesson.content} />; | ||
| return <MarkdownDisplay content={lesson.content} className="p-4 rounded-lg w-full" />; |
There was a problem hiding this comment.
Potential XSS risk with raw HTML.
Although MarkdownDisplay enhances readability, consider sanitizing lesson.content if it comes from untrusted sources. The rehypeRaw plugin processes raw HTML and could open the door for XSS attacks.
- return <MarkdownDisplay content={lesson.content} className="p-4 rounded-lg w-full" />;
+ // Recommendation: Apply a sanitize plugin or ensure you trust the source before using rehypeRawCommittable suggestion skipped: line range outside the PR's diff.
| </div> | ||
| <div className="w-[75vw] mx-auto mt-12 p-12 border-t-2 border-gray-300 max-tab:p-0 max-mob:p-0 max-tab:max-w-[100vw] max-mob:max-w-[100vw]"> | ||
| {draft?.content && <MDDisplay className="p-4 rounded-lg" source={draft.content} />} | ||
| {draft?.content && <MarkdownDisplay content={draft.content} className="p-4 rounded-lg" />} |
There was a problem hiding this comment.
Check for XSS vulnerabilities when displaying raw HTML.
If draft.content can contain user-generated or untrusted content, confirm that it is sanitized or that only trusted HTML is allowed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/content/courses/CourseSidebar.js (3)
7-17: Remove or Integrate theonCloseprop.
TheonCloseprop is never used in this component. You can either wire it into the closing logic or remove it to avoid confusion.
19-19: Avoid hardcodingnavbarHeight.
Extract the60value from a shared config or constant to keep the code more maintainable and consistent across components.
117-143: Consider unifying spacing offsets.
Currently, the top offset is set tonavbarHeight + 70. If possible, reference a shared layout constant or context to avoid mismatched spacing across components.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/content/courses/CourseSidebar.js(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/components/content/courses/CourseSidebar.js (5)
src/components/content/courses/CourseDetails.js (1)
useImageProxy(39-39)src/pages/course/[slug]/index.js (6)
navbarHeight(181-181)sidebarVisible(176-176)activeIndex(174-174)isMobileView(179-179)completedLessons(175-175)lessons(91-91)src/components/navbar/Navbar.js (1)
navbarHeight(19-19)src/hooks/useImageProxy.js (1)
returnImageProxy(6-26)src/components/buttons/GenericButton.js (1)
GenericButton(4-35)
🔇 Additional comments (6)
src/components/content/courses/CourseSidebar.js (6)
1-6: Imports look good.
21-26: Toggle function is clean and correct.
This logic properly relies on the parent’ssidebarVisiblestate without duplicating local state.
28-73: Verifylessonobject structure.
completedLessons.includes(lesson.id)implies eachlessonmust have a validid. Ensure you pass lessons with valid IDs to prevent unexpected errors.
75-99: Sidebar content component looks solid.
The close button logic for desktop mode is concise and user-friendly.
101-115: Mobile lessons tab is well-structured.
Rendering lessons in this separate layout is a clean solution for smaller screens.
145-145: Export statement is straightforward.
AustinKelsay
force-pushed
the
refactor/course-and-lesson-pages
branch
from
4a1675b to
55c2c34
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
src/pages/course/[slug]/index.js (1)
95-122: 🛠️ Refactor suggestionBatch lesson fetches to reduce re-renders
useLessonsfiresndk.fetchEventfor each lesson ID and callssetLessonsinside each promise, causingnre-renders. Fetch all in parallel and set state once to improve performance.-lessonIds.forEach(lessonId => fetchLesson(lessonId)); +Promise.all(lessonIds.map(fetchLesson)).then(() => {/* optionally sort here */});
♻️ Duplicate comments (1)
src/pages/course/[slug]/index.js (1)
411-421: Relay URL string duplication persists
The hard-coded relay list appears twice even after the earlier refactor suggestion. Extract it to a constant to stay DRY.
🧹 Nitpick comments (7)
src/components/content/courses/MobileCourseDetails.js (3)
23-24: Unusedroutervariable & import
useRouteris imported andconst router = useRouter()is executed butrouteris never referenced afterwards. This adds unnecessary bundle size and re-renders when the route changes.-import { useRouter } from 'next/router'; +// import { useRouter } from 'next/router'; // not needed @@ - const router = useRouter(); + // const router = useRouter(); // remove
31-35: Avoid usingindexas React list key
indexkeys break React’s reconciliation when the array order changes. The topic string itself is unique and stable – prefer that.- processedEvent.topics.map((topic, index) => ( - <Tag className="text-white" key={index} value={topic}></Tag> - )) + processedEvent.topics.map(topic => ( + <Tag className="text-white" key={topic} value={topic} /> + ))
86-90: Leverage optional chaining & reduce re-render workThe double check
processedEvent.description && …is safe but a bit verbose. Optional chaining is clearer and flagged by Biome.-processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>) +processedEvent.description?.split('\n').map((line, idx) => ( + <p key={idx} className="text-sm mb-2">{line}</p> +))🧰 Tools
🪛 Biome (1.9.4)
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/content/courses/DesktopCourseDetails.js (3)
1-5: UnuseduseRouterimport & variable
useRouteris imported and instantiated but never used. Remove to save bytes and avoid unnecessary renders.-import { useRouter } from 'next/router'; @@ - const router = useRouter(); + // router not required
63-68: Prefer stable key over array indexUsing
indexas key has reconciliation pitfalls; use the topic value.-processedEvent.topics.map((topic, index) => ( - <Tag className="text-white" key={index} value={topic}></Tag> -)) +processedEvent.topics.map(topic => ( + <Tag className="text-white" key={topic} value={topic} /> +))
102-105: Apply optional chaining to satisfy linter & improve readability-processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="mb-2">{line}</p>) +processedEvent.description?.split('\n').map((line, idx) => ( + <p key={idx} className="mb-2">{line}</p> +))🧰 Tools
🪛 Biome (1.9.4)
[error] 102-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/navbar/Navbar.js (1)
48-58: Specifykindsin NDK filter for faster relay queryFetching by
'#d'only may scan multiple kinds. If you know courses are kind 30023/30402, includekindsto limit relay load and reduce latency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (40)
package.json(2 hunks)src/components/banner/HeroBanner.js(1 hunks)src/components/charts/ActivityContributionChart.js(0 hunks)src/components/charts/CombinedContributionChart.js(0 hunks)src/components/content/carousels/CoursesCarousel.js(1 hunks)src/components/content/carousels/DocumentsCarousel.js(0 hunks)src/components/content/combined/CombinedDetails.js(3 hunks)src/components/content/courses/CombinedLesson.js(3 hunks)src/components/content/courses/CourseDetails.js(4 hunks)src/components/content/courses/CourseHeader.js(1 hunks)src/components/content/courses/CourseLesson.js(3 hunks)src/components/content/courses/CourseSidebar.js(1 hunks)src/components/content/courses/DesktopCourseDetails.js(1 hunks)src/components/content/courses/DocumentLesson.js(3 hunks)src/components/content/courses/DraftCourseDetails.js(2 hunks)src/components/content/courses/DraftCourseLesson.js(2 hunks)src/components/content/courses/MobileCourseDetails.js(1 hunks)src/components/content/courses/VideoLesson.js(3 hunks)src/components/content/documents/DocumentDetails.js(3 hunks)src/components/content/dropdowns/MessageDropdownItem.js(0 hunks)src/components/content/lists/ContentListItem.js(1 hunks)src/components/content/videos/VideoDetails.js(3 hunks)src/components/feeds/MessageInput.js(1 hunks)src/components/feeds/StackerNewsFeed.js(1 hunks)src/components/forms/course/embedded/EmbeddedDocumentForm.js(0 hunks)src/components/markdown/MarkdownDisplay.js(1 hunks)src/components/menutab/CommunityMenuTab.js(1 hunks)src/components/menutab/MenuTab.js(2 hunks)src/components/navbar/Navbar.js(3 hunks)src/components/profile/DataTables/UserProgressTable.js(0 hunks)src/components/profile/UserAccountLinking.js(0 hunks)src/components/profile/UserProfileCard.js(0 hunks)src/components/profile/subscription/CalendlyEmbed.js(1 hunks)src/components/zaps/ZapDisplay.js(1 hunks)src/components/zaps/ZapForm.js(1 hunks)src/config/appConfig.js(1 hunks)src/context/NDKContext.js(1 hunks)src/pages/_app.js(0 hunks)src/pages/course/[slug]/index.js(8 hunks)src/pages/draft/[slug]/index.js(2 hunks)
💤 Files with no reviewable changes (9)
- src/components/profile/UserAccountLinking.js
- src/components/forms/course/embedded/EmbeddedDocumentForm.js
- src/components/profile/DataTables/UserProgressTable.js
- src/components/profile/UserProfileCard.js
- src/components/charts/CombinedContributionChart.js
- src/components/content/carousels/DocumentsCarousel.js
- src/components/charts/ActivityContributionChart.js
- src/components/content/dropdowns/MessageDropdownItem.js
- src/pages/_app.js
✅ Files skipped from review due to trivial changes (8)
- src/config/appConfig.js
- src/components/feeds/StackerNewsFeed.js
- src/components/zaps/ZapForm.js
- src/components/feeds/MessageInput.js
- src/components/profile/subscription/CalendlyEmbed.js
- src/pages/draft/[slug]/index.js
- src/components/content/courses/VideoLesson.js
- src/components/content/courses/CourseLesson.js
🚧 Files skipped from review as they are similar to previous changes (19)
- src/components/menutab/CommunityMenuTab.js
- src/context/NDKContext.js
- src/components/content/carousels/CoursesCarousel.js
- src/components/content/lists/ContentListItem.js
- src/components/content/combined/CombinedDetails.js
- src/components/content/courses/DocumentLesson.js
- src/components/banner/HeroBanner.js
- src/components/zaps/ZapDisplay.js
- src/components/content/videos/VideoDetails.js
- src/components/content/courses/DraftCourseDetails.js
- src/components/markdown/MarkdownDisplay.js
- src/components/content/courses/CourseHeader.js
- src/components/content/documents/DocumentDetails.js
- src/components/content/courses/CourseDetails.js
- src/components/content/courses/DraftCourseLesson.js
- package.json
- src/components/content/courses/CourseSidebar.js
- src/components/content/courses/CombinedLesson.js
- src/components/menutab/MenuTab.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/content/courses/DesktopCourseDetails.js
[error] 102-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/components/content/courses/MobileCourseDetails.js
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
src/components/content/courses/DesktopCourseDetails.js (1)
10-14:decryptionPerformedprop appears unused
decryptionPerformedis accepted but never referenced inside this component. If not required, drop it from the signature to avoid confusion.src/pages/course/[slug]/index.js (2)
495-510: Possible mismatch between lesson classification andlesson.typeThe fall-through relies on both
lesson.typeandlesson.topics. EnsureparseEventsetslesson.typeconsistently; otherwise a lesson withtopics: ['video']but missingtypewould render nothing.
256-262: Course completion metric may be misleading
isCourseCompletedreturnstruewhen any lesson is completed. If full completion is intended, compare againstuniqueLessons.length.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/pages/course/[slug]/index.js (2)
412-413: Extract relay URLs to a constant or configuration.The relay URLs are still hardcoded in the component. This was previously identified in a past review and should be extracted to a constant or configuration value.
+ // Add at the top of the component or in a shared config file + const NOSTR_RELAYS = "wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/"; // Then in the component: <ZapThreadsWrapper anchor={nAddress} user={nsec || npub || null} - relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" + relays={NOSTR_RELAYS} disable="zaps" isAuthorized={isAuthorized} />
343-351: RefactorgetActiveTabIndexto reduce duplication withtoggleTab.The tab mapping logic is duplicated in both
getActiveTabIndexandtoggleTabfunctions, which was previously noted as an issue. Consider refactoring to define the tab map once and reuse it in both functions.+ // Define tabMap once at component level + const getTabMap = useCallback(() => { + const tabMap = ['overview', 'content', 'qa']; + if (isMobileView) { + tabMap.splice(2, 0, 'lessons'); + } + return tabMap; + }, [isMobileView]); const toggleTab = (index) => { - const tabMap = ['overview', 'content', 'qa']; - // If mobile and we have the lessons tab, insert it at index 2 - if (isMobileView) { - tabMap.splice(2, 0, 'lessons'); - } + const tabMap = getTabMap(); const tabName = tabMap[index]; setActiveTab(tabName); // Only show/hide sidebar on mobile - desktop keeps sidebar visible if (isMobileView) { // Rest of the function... } }; // Map active tab name back to index for MenuTab const getActiveTabIndex = () => { - const tabMap = ['overview', 'content', 'qa']; - if (isMobileView) { - tabMap.splice(2, 0, 'lessons'); - } + const tabMap = getTabMap(); return tabMap.indexOf(activeTab); };
🧹 Nitpick comments (4)
src/pages/course/[slug]/index.js (4)
604-606: Fix incorrect tab label reference.When selecting a lesson on mobile, the code attempts to find a tab labeled 'Content' which is correct but could be more robust by using the same constant defined in the tab items.
onLessonSelect={(index) => { handleLessonSelect(index); if (isMobileView) { - toggleTab(getTabItems().findIndex(item => item.label === 'Content')); + toggleTab(1); // Content tab is always at index 1 in getTabItems() } }}
514-621: Consider extracting layout components for better maintainability.The component contains a large render method with complex conditional rendering and styling. Consider extracting parts of the UI into smaller components to improve readability and maintainability.
For example, you could create:
- A
CourseTabContentcomponent for tab content rendering- A
SidebarContainercomponent that handles the different desktop/mobile sidebar behaviorsThis would make the main component more focused on orchestrating the app flow rather than handling complex UI rendering details.
216-217: Add dependency onrouter.queryfor sidebar visibility useEffect.The useEffect hook sets sidebar visibility based on
isMobileView, but should also react to changes in therouter.queryobject to handle cases where the URL is updated directly.useEffect(() => { // Auto-open sidebar on desktop, close on mobile setSidebarVisible(!isMobileView); - }, [isMobileView]); + }, [isMobileView, router.query]);
285-290: Extract authorization logic to a function or hook.The
isAuthorizedcalculation is used in multiple places and could be extracted to a reusable function or custom hook for better maintainability.+ const useAuthorization = (session, course, paidCourse) => { + return useMemo(() => { + return session?.user?.role?.subscribed || + session?.user?.pubkey === course?.pubkey || + !paidCourse || + session?.user?.purchased?.some(purchase => purchase.courseId === course?.d); + }, [session, course, paidCourse]); + }; // Then in the component: - const isAuthorized = - session?.user?.role?.subscribed || - session?.user?.pubkey === course?.pubkey || - !paidCourse || - session?.user?.purchased?.some(purchase => purchase.courseId === course?.d) + const isAuthorized = useAuthorization(session, course, paidCourse);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/config/appConfig.js(1 hunks)src/pages/course/[slug]/index.js(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/appConfig.js
🔇 Additional comments (3)
src/pages/course/[slug]/index.js (3)
383-400: Improve keyboard navigation accessibility.The keyboard navigation for tabs has been implemented as requested, which is great. Consider adding ARIA roles and additional keyboard support for improved accessibility.
The implementation of keyboard navigation for tabs is now in place, making the interface more accessible to keyboard users. Good job on implementing this accessibility improvement!
1-21: Good component organization with appropriate imports.The imports are well-organized, and the new components like
CourseSidebar,CourseHeader, andMarkdownDisplayimprove modularity and reusability.The refactoring to use the new
MarkdownDisplaycomponent instead of dynamic imports is a good architectural improvement.
531-564: Well-implemented tab content rendering with conditional display.The main content area is well-structured with proper conditional rendering for different tabs and responsive layout considerations.
The use of conditional class application (
${activeTab === 'overview' ? 'block' : 'hidden'}) provides a clean way to toggle tab content visibility.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/pages/course/[slug]/index.js (1)
319-337: 🛠️ Refactor suggestionRefactor tab mapping logic to prevent duplication
The tab mapping logic is duplicated in both
toggleTabandgetActiveTabIndex(lines 344-351), which can lead to maintenance issues if tab order changes.+// Define tab map once at the component level +const getTabMap = useCallback(() => { + const tabMap = ['overview', 'content', 'qa']; + if (isMobileView) { + tabMap.splice(2, 0, 'lessons'); + } + return tabMap; +}, [isMobileView]); const toggleTab = (index) => { - const tabMap = ['overview', 'content', 'qa']; - // If mobile and we have the lessons tab, insert it at index 2 - if (isMobileView) { - tabMap.splice(2, 0, 'lessons'); - } + const tabMap = getTabMap(); const tabName = tabMap[index]; setActiveTab(tabName); // Only show/hide sidebar on mobile - desktop keeps sidebar visible if (isMobileView) { if (tabName === 'lessons') { setSidebarVisible(true); } else { setSidebarVisible(false); } } }; // Also update getActiveTabIndex to use the shared function const getActiveTabIndex = () => { - const tabMap = ['overview', 'content', 'qa']; - if (isMobileView) { - tabMap.splice(2, 0, 'lessons'); - } + const tabMap = getTabMap(); return tabMap.indexOf(activeTab); };
🧹 Nitpick comments (4)
src/components/content/courses/MobileCourseDetails.js (1)
86-90: Simplify description rendering with optional chainingThe description rendering logic can be simplified using optional chaining and nullish coalescing.
-{processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)} +{processedEvent.description?.split('\n') + .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>) || null}🧰 Tools
🪛 Biome (1.9.4)
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/pages/course/[slug]/index.js (3)
410-415: Extract relay URLs to a constantThe relay URLs are hardcoded in this component. This should be extracted to a constant or configuration to avoid duplication and simplify maintenance.
+// Add at the top of the component or in a shared config file +const NOSTR_RELAYS = "wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/"; // Then in the component: <ZapThreadsWrapper anchor={nAddress} user={nsec || npub || null} - relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" + relays={NOSTR_RELAYS} disable="zaps" isAuthorized={isAuthorized} />
353-380: Memoize tab items for better performanceThe
getTabItemsfunction is called multiple times, including in render cycles. Consider usinguseMemoto prevent unnecessary recalculations.- // Create tab items for MenuTab - const getTabItems = () => { - const items = [ - { - label: 'Overview', - icon: 'pi pi-home', - }, - { - label: 'Content', - icon: 'pi pi-book', - } - ]; - - // Add lessons tab only on mobile - if (isMobileView) { - items.push({ - label: 'Lessons', - icon: 'pi pi-list', - }); - } - - items.push({ - label: 'Comments', - icon: 'pi pi-comments', - }); - - return items; - }; + // Memoize tab items + const tabItems = useMemo(() => { + const items = [ + { + label: 'Overview', + icon: 'pi pi-home', + }, + { + label: 'Content', + icon: 'pi pi-book', + } + ]; + + // Add lessons tab only on mobile + if (isMobileView) { + items.push({ + label: 'Lessons', + icon: 'pi pi-list', + }); + } + + items.push({ + label: 'Comments', + icon: 'pi pi-comments', + }); + + return items; + }, [isMobileView]);Then update all references to
getTabItems()to usetabItemsdirectly.
183-187: Consider using a constant for the mobile breakpointThe mobile viewport breakpoint is hardcoded as 968. Consider extracting this to a named constant or configuration value for better maintainability.
+ const MOBILE_BREAKPOINT = 968; // Extract to a constant const [sidebarVisible, setSidebarVisible] = useState(false); const [nAddress, setNAddress] = useState(null); const windowWidth = useWindowWidth(); - const isMobileView = windowWidth <= 968; + const isMobileView = windowWidth <= MOBILE_BREAKPOINT;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/components/content/courses/CombinedLesson.js(4 hunks)src/components/content/courses/CourseDetails.js(4 hunks)src/components/content/courses/MobileCourseDetails.js(1 hunks)src/components/content/courses/VideoLesson.js(4 hunks)src/components/content/videos/VideoDetails.js(4 hunks)src/components/navbar/Navbar.js(3 hunks)src/pages/course/[slug]/index.js(7 hunks)src/pages/index.js(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/pages/index.js
- src/components/content/courses/VideoLesson.js
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/content/videos/VideoDetails.js
- src/components/content/courses/CombinedLesson.js
- src/components/content/courses/CourseDetails.js
- src/components/navbar/Navbar.js
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/pages/course/[slug]/index.js (6)
src/components/content/courses/CourseDetails.js (4)
nAddress(37-37)windowWidth(43-43)session(41-41)CourseDetails(25-233)src/hooks/useWindowWidth.js (1)
useWindowWidth(3-24)src/components/content/courses/CourseSidebar.js (2)
navbarHeight(19-19)CourseSidebar(7-143)src/components/ZapThreadsWrapper.js (1)
ZapThreadsWrapper(3-92)src/components/menutab/MenuTab.js (1)
MenuTab(5-44)src/components/markdown/MarkdownDisplay.js (1)
MarkdownDisplay(8-25)
🪛 Biome (1.9.4)
src/components/content/courses/MobileCourseDetails.js
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
src/components/content/courses/MobileCourseDetails.js (4)
8-21: Props structure looks goodThe component accepts all necessary props for rendering a comprehensive mobile course view, including content details, payment status, author information, and UI control flags.
33-35: Completion tag implementation is correctThe conditional rendering logic for the completion tag properly respects the
showCompletedTagprop, fixing the issue mentioned in previous reviews.
60-67: Author link is properly implementedThe author link now correctly includes the href attribute, making it functional as a navigation link to the author's profile page.
98-131: Course information section is well structuredThe course information section provides a clear overview of course details with appropriate conditional rendering for paid courses and publication dates.
src/pages/course/[slug]/index.js (4)
554-556: MarkdownDisplay refactoring is well implementedThe refactoring to use the new
MarkdownDisplaycomponent for rendering markdown content is well implemented and consistent with the broader application changes.
382-400: Keyboard navigation enhancement is well implementedThe implementation of keyboard navigation for tabs using arrow keys is well done and improves accessibility.
428-466: Course overview section with responsive designThe course overview section is well implemented with responsive design considerations, showing different layouts for mobile and desktop views.
603-607:⚠️ Potential issueFix incorrect tab label reference
When selecting a lesson on mobile, the code attempts to find a tab with the label 'Content', but should verify this matches the exact label used in
getTabItems().onLessonSelect={(index) => { handleLessonSelect(index); if (isMobileView) { - toggleTab(getTabItems().findIndex(item => item.label === 'Content')); + toggleTab(1); // Directly use index 1 which corresponds to 'Content' tab } }}Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/content/courses/DesktopCourseDetails.js (1)
33-33: Add null/undefined handling for returnImageProxy function callsThe
returnImageProxyfunction is called without checking if the parameters might be undefined. Consider adding null handling or ensuring that default values are provided.- src={returnImageProxy(processedEvent.image)} + src={returnImageProxy(processedEvent?.image || '')}- src={returnImageProxy(author?.avatar, author?.pubkey)} + src={returnImageProxy(author?.avatar || '', author?.pubkey || '')}Also applies to: 75-75
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/content/courses/DesktopCourseDetails.js(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/content/courses/DesktopCourseDetails.js
[error] 103-106: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/components/content/courses/DesktopCourseDetails.js (1)
1-152: Overall, well-structured component with good organizationThe component is well-organized with clear section comments, proper use of conditional rendering, and good accessibility practices. The layout is responsive and follows good React component design principles.
🧰 Tools
🪛 Biome (1.9.4)
[error] 103-106: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
…inst invalid slug in navbar
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
AustinKelsay
force-pushed
the
refactor/course-and-lesson-pages
branch
from
35639b5 to
cc86708
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/pages/course/[slug]/index.js (4)
411-412: Extract relay URLs to a constant.The relay URLs are hardcoded in the component. Consider extracting them to a constant or configuration to maintain DRY principles.
+ // At the top of the file with other constants + const NOSTR_RELAYS = "wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/"; // Then in the component <ZapThreadsWrapper anchor={nAddress} user={nsec || npub || null} - relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" + relays={NOSTR_RELAYS} disable="zaps" isAuthorized={isAuthorized} />
334-335:⚠️ Potential issueFix incorrect reference to getTabMap.
There's a bug in the tab toggling logic where
getTabMapis incorrectly treated as an array instead of a memoized value.const toggleTab = (index) => { - const tabName = getTabMap[index]; + const tabName = getTabMap[index]; setActiveTab(tabName);
347-350:⚠️ Potential issueInconsistent approach to accessing tab map.
The implementation of
getActiveTabIndexincorrectly usesindexOfongetTabMapdirectly, which treats it as an array, contradicting its implementation as a memoized value.const getActiveTabIndex = () => { - return getTabMap.indexOf(activeTab); + return getTabMap.indexOf(activeTab); };
382-399:⚠️ Potential issueFix keyboard navigation accessibility implementation.
The keyboard navigation implementation also incorrectly accesses
getTabMapas an array instead of a memoized value.useEffect(() => { const handleKeyDown = (e) => { if (e.key === 'ArrowRight') { const currentIndex = getActiveTabIndex(); - const nextIndex = (currentIndex + 1) % getTabMap.length; + const nextIndex = (currentIndex + 1) % getTabMap.length; toggleTab(nextIndex); } else if (e.key === 'ArrowLeft') { const currentIndex = getActiveTabIndex(); - const prevIndex = (currentIndex - 1 + getTabMap.length) % getTabMap.length; + const prevIndex = (currentIndex - 1 + getTabMap().length) % getTabMap().length; toggleTab(prevIndex); } }; document.addEventListener('keydown', handleKeyDown); return () => { document.removeEventListener('keydown', handleKeyDown); }; }, [activeTab, getTabMap, toggleTab]);
🧹 Nitpick comments (2)
src/components/content/courses/MobileCourseDetails.js (2)
86-89: Improve robustness with optional chaining.The description rendering code could be more resilient to handle cases where
processedEvent.descriptionmight be undefined or null.- {processedEvent.description && - processedEvent.description - .split('\n') - .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)} + {processedEvent.description?.split('\n') + .map((line, index) => <p key={index} className="text-sm mb-2">{line}</p>)}🧰 Tools
🪛 Biome (1.9.4)
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
27-36: Consider adding link functionality to topic tags.The topic tags are currently displayed but not clickable. Consider making them function as links to filter courses by topic.
{processedEvent.topics && processedEvent.topics.length > 0 && processedEvent.topics.map((topic, index) => ( - <Tag className="text-white" key={index} value={topic}></Tag> + <Tag + className="text-white cursor-pointer" + key={index} + value={topic} + onClick={() => router.push(`/courses?topic=${encodeURIComponent(topic)}`)} + ></Tag> ))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (41)
package.json(2 hunks)src/components/banner/HeroBanner.js(1 hunks)src/components/charts/ActivityContributionChart.js(0 hunks)src/components/charts/CombinedContributionChart.js(0 hunks)src/components/content/carousels/CoursesCarousel.js(1 hunks)src/components/content/carousels/DocumentsCarousel.js(0 hunks)src/components/content/combined/CombinedDetails.js(3 hunks)src/components/content/courses/CombinedLesson.js(4 hunks)src/components/content/courses/CourseDetails.js(4 hunks)src/components/content/courses/CourseHeader.js(1 hunks)src/components/content/courses/CourseLesson.js(3 hunks)src/components/content/courses/CourseSidebar.js(1 hunks)src/components/content/courses/DesktopCourseDetails.js(1 hunks)src/components/content/courses/DocumentLesson.js(3 hunks)src/components/content/courses/DraftCourseDetails.js(2 hunks)src/components/content/courses/DraftCourseLesson.js(2 hunks)src/components/content/courses/MobileCourseDetails.js(1 hunks)src/components/content/courses/VideoLesson.js(4 hunks)src/components/content/documents/DocumentDetails.js(3 hunks)src/components/content/dropdowns/MessageDropdownItem.js(0 hunks)src/components/content/lists/ContentListItem.js(1 hunks)src/components/content/videos/VideoDetails.js(4 hunks)src/components/feeds/MessageInput.js(1 hunks)src/components/feeds/StackerNewsFeed.js(1 hunks)src/components/forms/course/embedded/EmbeddedDocumentForm.js(0 hunks)src/components/markdown/MarkdownDisplay.js(1 hunks)src/components/menutab/CommunityMenuTab.js(1 hunks)src/components/menutab/MenuTab.js(1 hunks)src/components/navbar/Navbar.js(3 hunks)src/components/profile/DataTables/UserProgressTable.js(0 hunks)src/components/profile/UserAccountLinking.js(0 hunks)src/components/profile/UserProfileCard.js(0 hunks)src/components/profile/subscription/CalendlyEmbed.js(1 hunks)src/components/zaps/ZapDisplay.js(1 hunks)src/components/zaps/ZapForm.js(1 hunks)src/config/appConfig.js(1 hunks)src/context/NDKContext.js(1 hunks)src/pages/_app.js(0 hunks)src/pages/course/[slug]/index.js(7 hunks)src/pages/draft/[slug]/index.js(2 hunks)src/pages/index.js(1 hunks)
💤 Files with no reviewable changes (9)
- src/components/profile/UserAccountLinking.js
- src/components/content/carousels/DocumentsCarousel.js
- src/components/forms/course/embedded/EmbeddedDocumentForm.js
- src/pages/_app.js
- src/components/profile/UserProfileCard.js
- src/components/profile/DataTables/UserProgressTable.js
- src/components/charts/CombinedContributionChart.js
- src/components/charts/ActivityContributionChart.js
- src/components/content/dropdowns/MessageDropdownItem.js
✅ Files skipped from review due to trivial changes (5)
- src/components/feeds/MessageInput.js
- src/config/appConfig.js
- src/context/NDKContext.js
- src/components/banner/HeroBanner.js
- src/components/content/courses/VideoLesson.js
🚧 Files skipped from review as they are similar to previous changes (25)
- src/components/menutab/CommunityMenuTab.js
- src/pages/index.js
- src/components/zaps/ZapDisplay.js
- src/components/content/carousels/CoursesCarousel.js
- src/components/feeds/StackerNewsFeed.js
- src/components/content/courses/DraftCourseLesson.js
- src/components/zaps/ZapForm.js
- src/components/content/lists/ContentListItem.js
- src/components/content/combined/CombinedDetails.js
- src/components/content/courses/CourseSidebar.js
- package.json
- src/components/content/courses/DocumentLesson.js
- src/components/content/documents/DocumentDetails.js
- src/components/profile/subscription/CalendlyEmbed.js
- src/components/content/courses/CombinedLesson.js
- src/components/content/courses/DesktopCourseDetails.js
- src/components/content/videos/VideoDetails.js
- src/components/content/courses/CourseHeader.js
- src/components/menutab/MenuTab.js
- src/components/navbar/Navbar.js
- src/components/markdown/MarkdownDisplay.js
- src/pages/draft/[slug]/index.js
- src/components/content/courses/CourseDetails.js
- src/components/content/courses/CourseLesson.js
- src/components/content/courses/DraftCourseDetails.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/content/courses/MobileCourseDetails.js
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
src/components/content/courses/MobileCourseDetails.js (3)
63-64: Good implementation of author link.The anchor tag for the author name now correctly includes the href attribute, addressing a previous issue and making the link functional.
33-35: Good implementation ofshowCompletedTagprop.The completed tag is now correctly conditional on both the
isCompletedstate and theshowCompletedTagprop.
1-135: Well-implemented mobile responsive component.This component implements a clean, mobile-optimized layout for course details with appropriate spacing, information hierarchy, and conditional rendering based on course properties.
🧰 Tools
🪛 Biome (1.9.4)
[error] 86-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/pages/course/[slug]/index.js (6)
586-589: Fix lesson selection handling on mobile.The code now correctly sets the active tab directly instead of searching for it by label, addressing a previous issue.
190-198: Good implementation of memoized tab map.The tab map is now properly memoized with useMemo and conditionally includes the lessons tab for mobile view, improving performance and code organization.
200-213: Good handling of missing naddress.The implementation now properly handles cases where the slug doesn't contain an naddress, showing an error toast and redirecting to the courses page after a delay.
515-527: Well-implemented tab navigation.The tab navigation using the MenuTab component is well-structured and handles both mobile and desktop views appropriately.
531-624: Good responsive layout implementation.The layout structure effectively handles both mobile and desktop views with appropriate sidebar behavior, fixed width calculations, and smooth transitions.
552-556: Good use of MarkdownDisplay component.The code now uses the new MarkdownDisplay component for rendering markdown content, which aligns with the pull request's overall goal of standardizing markdown rendering.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/pages/course/[slug]/index.js (4)
340-342:⚠️ Potential issueFix incorrect reference to getTabMap.
There's a bug in the tab toggling logic where
getTabMapis incorrectly treated as an array instead of a function.const toggleTab = (index) => { - const tabName = getTabMap[index]; + const tabName = getTabMap()[index]; setActiveTab(tabName);
354-357:⚠️ Potential issueInconsistent approach to accessing tab map.
The implementation of
getActiveTabIndexinconsistently usesindexOfongetTabMapdirectly, which treats it as an array, contradicting its implementation as a function.const getActiveTabIndex = () => { - return getTabMap.indexOf(activeTab); + return getTabMap().indexOf(activeTab); };
389-406:⚠️ Potential issueFix keyboard navigation implementation.
The keyboard navigation implementation also treats
getTabMapas an array rather than calling it as a function.useEffect(() => { const handleKeyDown = (e) => { if (e.key === 'ArrowRight') { const currentIndex = getActiveTabIndex(); - const nextIndex = (currentIndex + 1) % getTabMap.length; + const nextIndex = (currentIndex + 1) % getTabMap().length; toggleTab(nextIndex); } else if (e.key === 'ArrowLeft') { const currentIndex = getActiveTabIndex(); - const prevIndex = (currentIndex - 1 + getTabMap.length) % getTabMap.length; + const prevIndex = (currentIndex - 1 + getTabMap().length) % getTabMap().length; toggleTab(prevIndex); } }; document.addEventListener('keydown', handleKeyDown); return () => { document.removeEventListener('keydown', handleKeyDown); }; }, [activeTab, getTabMap, toggleTab]);
415-419: 🛠️ Refactor suggestionExtract relay URLs to a constant or configuration.
The relay URLs are hardcoded in the ZapThreadsWrapper component. Consider extracting them to a constant or configuration value to maintain consistency and avoid duplication.
+ // Add at the top of the component or in a shared config file + const NOSTR_RELAYS = "wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/"; <ZapThreadsWrapper anchor={nAddress} user={nsec || npub || null} - relays="wss://nos.lol/, wss://relay.damus.io/, wss://relay.snort.social/, wss://relay.nostr.band/, wss://relay.primal.net/, wss://nostrue.com/, wss://purplerelay.com/, wss://relay.devs.tools/" + relays={NOSTR_RELAYS} disable="zaps" isAuthorized={isAuthorized} />
🧹 Nitpick comments (1)
src/pages/course/[slug]/index.js (1)
131-136: Redundant deduplication logic.There's duplicate deduplication logic. The code first checks for duplicates when adding lessons to the newLessons array (lines 115-118), and then does another deduplication step here using a Map.
- useEffect(() => { - const newUniqueLessons = Array.from( - new Map(lessons.map(lesson => [lesson.id, lesson])).values() - ); - setUniqueLessons(newUniqueLessons); - }, [lessons]); + useEffect(() => { + // lessons are already deduplicated in the fetchLessons function + setUniqueLessons(lessons); + }, [lessons]);Alternatively, you could remove the deduplication in the fetchLessons function and keep only this one.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/content/courses/DesktopCourseDetails.js(1 hunks)src/pages/course/[slug]/index.js(8 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/content/courses/DesktopCourseDetails.js
🔇 Additional comments (6)
src/pages/course/[slug]/index.js (6)
93-128: Improved lesson fetching efficiency with batch request.The refactored lesson fetching logic now makes a single network request with all lesson IDs instead of multiple individual requests. This is a significant performance improvement, especially for courses with many lessons.
197-205: Good use of useMemo for tab configuration.Using useMemo for the tabMap calculation is efficient, as it only recalculates when the view mode changes.
207-220: Proper implementation of nAddress error handling.The previously missing "no naddress" error handling has been properly implemented with error notification and redirection.
520-535: Good implementation of tab navigation.The MenuTab implementation provides a clean and responsive navigation system with proper sidebar controls for mobile views.
561-562: Switched to new MarkdownDisplay component.The code now uses the new MarkdownDisplay component with the updated
contentprop instead of the previous dynamic import for markdown rendering.
572-630: Well-structured responsive sidebar implementation.The sidebar implementation handles both mobile and desktop views appropriately, with smooth transitions and proper visibility controls.
1 participant
Summary by CodeRabbit
New Features
MarkdownDisplaycomponent for consistent and enhanced markdown rendering with improved styling and support for GitHub Flavored Markdown.CourseSidebarcomponent for responsive lesson navigation on desktop and mobile.CourseHeadercomponent to provide improved course navigation context.DesktopCourseDetailsandMobileCourseDetailscomponents for detailed and responsive course information display.Refactor
Chores
github-markdown-cssandremark-breaksdependencies to enhance markdown styling and line break handling.@uiw/react-markdown-previewdependency and its associated CSS import.nextdependency to the latest patch version for improved stability and performance.