Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

1071 video player page for courses #1176

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 131 additions & 0 deletions app/(app)/courses/[slug]/[id]/_client.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
"use client";
import { FEATURE_FLAGS, isFlagEnabled } from "@/utils/flags";
import { CircleCheck, SquarePlay } from "lucide-react";
import { type Session } from "next-auth";
import { notFound } from "next/navigation";
import { mockContentList } from "../../mock";

interface ContentProps {
session: Session | null;
}
Comment on lines +8 to +10
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Remove unused session prop

The session prop is defined in the interface but is not used anywhere within the Content component implementation. The component only uses feature flag checks and renders child components without any session-dependent functionality.

  • Remove the session prop from the ContentProps interface
  • Remove the session parameter from the Content component definition
🔗 Analysis chain

Verify usage of session prop

The session prop is defined in the interface but doesn't appear to be used within the Content component. Either remove the unused prop or implement the intended session-dependent functionality.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for session usage in the component
ast-grep --pattern 'session.$$$' 

Length of output: 10367


Script:

#!/bin/bash
# Let's search for the Content component implementation and session usage
ast-grep --pattern 'const Content = ({ session }: ContentProps) => {
  $$$
}'

Length of output: 1434


const Content = ({ session }: ContentProps) => {
const flagEnabled = isFlagEnabled(FEATURE_FLAGS.COURSE_VIDEO);

if (!flagEnabled) {
notFound();
}

return (
<div className="overflow-auto">
<div className="w-full divide-x divide-gray-700 lg:grid lg:grid-cols-12">
<VideoPlayer />
<Sidebar contentList={mockContentList} />
</div>
<div className="mx-auto max-w-5xl">
<ContentList contentList={mockContentList} />
</div>
</div>
);
};
Comment on lines +12 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error boundaries and loading states

The component should handle potential errors from mock data and add loading states for better UX. Consider implementing:

  1. Error boundaries to catch rendering errors
  2. Loading states while checking feature flags
  3. Proper error handling for missing or malformed mock data

Here's a suggested implementation:

 const Content = ({ session }: ContentProps) => {
+  const [isLoading, setIsLoading] = useState(true);
   const flagEnabled = isFlagEnabled(FEATURE_FLAGS.COURSE_VIDEO);
 
   if (!flagEnabled) {
     notFound();
   }
 
+  if (isLoading) {
+    return <div>Loading...</div>;
+  }
+
+  if (!mockContentList?.length) {
+    return <div>No content available</div>;
+  }
+
   return (
     <div className="overflow-auto">

Committable suggestion was skipped due to low confidence.


const VideoPlayer = () => (
<div className="col-span-9">
<div className="mx-auto w-full max-w-7xl bg-black">
<iframe
className="aspect-video w-full"
src="https://www.youtube.com/embed/oKKG-CpDjrI"
title="YouTube video"
></iframe>
</div>
</div>
);
Comment on lines +32 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance iframe security and performance

The YouTube iframe implementation needs security and performance improvements:

Apply these security and performance enhancements:

 const VideoPlayer = () => (
   <div className="col-span-9">
     <div className="mx-auto w-full max-w-7xl bg-black">
       <iframe
         className="aspect-video w-full"
         src="https://www.youtube.com/embed/oKKG-CpDjrI"
         title="YouTube video"
+        loading="lazy"
+        sandbox="allow-scripts allow-same-origin allow-presentation"
+        allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
+        referrerPolicy="strict-origin-when-cross-origin"
       ></iframe>
     </div>
   </div>
 );

Also consider:

  1. Making the video URL configurable through props
  2. Adding error handling for failed video loads
  3. Implementing a loading placeholder
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const VideoPlayer = () => (
<div className="col-span-9">
<div className="mx-auto w-full max-w-7xl bg-black">
<iframe
className="aspect-video w-full"
src="https://www.youtube.com/embed/oKKG-CpDjrI"
title="YouTube video"
></iframe>
</div>
</div>
);
const VideoPlayer = () => (
<div className="col-span-9">
<div className="mx-auto w-full max-w-7xl bg-black">
<iframe
className="aspect-video w-full"
src="https://www.youtube.com/embed/oKKG-CpDjrI"
title="YouTube video"
loading="lazy"
sandbox="allow-scripts allow-same-origin allow-presentation"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
referrerPolicy="strict-origin-when-cross-origin"
></iframe>
</div>
</div>
);


interface SidebarProps {
contentList: typeof mockContentList;
}

const Sidebar = ({ contentList }: SidebarProps) => (
<div className="col-span-3 flex w-full flex-col overflow-auto">
<ul className="divide-y divide-gray-700">
{contentList && contentList.length > 0 ? (
contentList.map((item, index) => (
<SidebarItem key={index} item={item} />
))
) : (
<li className="text-center text-gray-500">No content available</li>
)}
</ul>
</div>
);

interface SidebarItemProps {
item: (typeof mockContentList)[0];
}

const SidebarItem = ({ item }: SidebarItemProps) => (
<li className="flex flex-row items-center justify-between px-2 py-2">
<div className="flex flex-row items-center">
<SquarePlay
className="mr-4 h-5 w-5 text-white group-hover:text-white"
aria-hidden="true"
/>
<p>{item.title}</p>
</div>
<CircleCheck
className={`h-6 w-6 ${
item.watched ? "mr-2 h-5 w-5 text-pink-600" : "mr-2 h-5 w-5 text-white"
}`}
aria-hidden="true"
/>
</li>
);
Comment on lines +44 to +82
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve accessibility and type definitions

The sidebar implementation needs accessibility enhancements and better type definitions:

  1. Replace mock data type with proper interface:
interface ContentItem {
  id: string;
  title: string;
  watched: boolean;
  // ... other properties
}

interface SidebarProps {
  contentList: ContentItem[];
}
  1. Enhance accessibility:
-<ul className="divide-y divide-gray-700">
+<ul 
+  className="divide-y divide-gray-700"
+  role="list"
+  aria-label="Course content"
+>

-<li className="flex flex-row items-center justify-between px-2 py-2">
+<li 
+  className="flex flex-row items-center justify-between px-2 py-2"
+  tabIndex={0}
+  role="listitem"
+>


interface ContentListProps {
contentList: typeof mockContentList;
}

const ContentList = ({ contentList }: ContentListProps) => {
return (
<>
{contentList.map(
({
id,
title,
longDescription,
description,
publishedDate,
author,
imageLink,
}) => (
<div key={id} className="mx-auto w-full max-w-7xl py-8">
<h2 className="text-3xl font-extrabold tracking-tight text-neutral-800 dark:text-white">
{title}
</h2>
<li key={author} className="flex justify-between gap-x-6 py-5">
<div className="flex min-w-0 gap-x-4">
<img
alt=""
src={imageLink}
className="h-12 w-12 flex-none rounded-full bg-gray-50"
/>
<div className="min-w-0 flex-auto">
<p className="text-sm font-semibold leading-6 text-neutral-800 dark:text-white">
{author}
</p>
<p className="mt-1 truncate text-xs leading-5 text-gray-500">
{publishedDate}
</p>
</div>
</div>
</li>
<div>{description}</div>
<div>{longDescription}</div>
</div>
),
)}
</>
);
};
Comment on lines +88 to +129
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix HTML structure and accessibility issues

The ContentList component has several structural and accessibility issues:

Apply these fixes:

 const ContentList = ({ contentList }: ContentListProps) => {
   return (
-    <>
+    <div className="content-list" role="region" aria-label="Course content details">
       {contentList.map(
         ({
           id,
           title,
           longDescription,
           description,
           publishedDate,
           author,
           imageLink,
         }) => (
           <div key={id} className="mx-auto w-full max-w-7xl py-8">
             <h2 className="text-3xl font-extrabold tracking-tight text-neutral-800 dark:text-white">
               {title}
             </h2>
-            <li key={author} className="flex justify-between gap-x-6 py-5">
+            <div key={author} className="flex justify-between gap-x-6 py-5">
               <div className="flex min-w-0 gap-x-4">
                 <img
-                  alt=""
+                  alt={`${author}'s profile picture`}
                   src={imageLink}
                   className="h-12 w-12 flex-none rounded-full bg-gray-50"
                 />
                 <div className="min-w-0 flex-auto">
                   <p className="text-sm font-semibold leading-6 text-neutral-800 dark:text-white">
                     {author}
                   </p>
                   <p className="mt-1 truncate text-xs leading-5 text-gray-500">
                     {publishedDate}
                   </p>
                 </div>
               </div>
-            </li>
+            </div>
-            <div>{description}</div>
-            <div>{longDescription}</div>
+            <div role="article">
+              <h3 className="sr-only">Course description</h3>
+              <p>{description}</p>
+              <p>{longDescription}</p>
+            </div>
           </div>
         ),
       )}
-    </>
+    </div>
   );
 };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const ContentList = ({ contentList }: ContentListProps) => {
return (
<>
{contentList.map(
({
id,
title,
longDescription,
description,
publishedDate,
author,
imageLink,
}) => (
<div key={id} className="mx-auto w-full max-w-7xl py-8">
<h2 className="text-3xl font-extrabold tracking-tight text-neutral-800 dark:text-white">
{title}
</h2>
<li key={author} className="flex justify-between gap-x-6 py-5">
<div className="flex min-w-0 gap-x-4">
<img
alt=""
src={imageLink}
className="h-12 w-12 flex-none rounded-full bg-gray-50"
/>
<div className="min-w-0 flex-auto">
<p className="text-sm font-semibold leading-6 text-neutral-800 dark:text-white">
{author}
</p>
<p className="mt-1 truncate text-xs leading-5 text-gray-500">
{publishedDate}
</p>
</div>
</div>
</li>
<div>{description}</div>
<div>{longDescription}</div>
</div>
),
)}
</>
);
};
const ContentList = ({ contentList }: ContentListProps) => {
return (
<div className="content-list" role="region" aria-label="Course content details">
{contentList.map(
({
id,
title,
longDescription,
description,
publishedDate,
author,
imageLink,
}) => (
<div key={id} className="mx-auto w-full max-w-7xl py-8">
<h2 className="text-3xl font-extrabold tracking-tight text-neutral-800 dark:text-white">
{title}
</h2>
<div key={author} className="flex justify-between gap-x-6 py-5">
<div className="flex min-w-0 gap-x-4">
<img
alt={`${author}'s profile picture`}
src={imageLink}
className="h-12 w-12 flex-none rounded-full bg-gray-50"
/>
<div className="min-w-0 flex-auto">
<p className="text-sm font-semibold leading-6 text-neutral-800 dark:text-white">
{author}
</p>
<p className="mt-1 truncate text-xs leading-5 text-gray-500">
{publishedDate}
</p>
</div>
</div>
</div>
<div role="article">
<h3 className="sr-only">Course description</h3>
<p>{description}</p>
<p>{longDescription}</p>
</div>
</div>
),
)}
</div>
);
};


export default Content;
13 changes: 13 additions & 0 deletions app/(app)/courses/[slug]/[id]/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { getServerAuthSession } from "@/server/auth";
import Content from "./_client";
Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding loading and error states.

The page lacks loading and error handling components which are important for better UX.

Add loading.tsx and error.tsx files in the same directory:

// loading.tsx
export default function Loading() {
  return <div>Loading course content...</div>;
}

// error.tsx
export default function Error({
  error,
  reset,
}: {
  error: Error;
  reset: () => void;
}) {
  return (
    <div>
      <h2>Something went wrong!</h2>
      <button onClick={reset}>Try again</button>
    </div>
  );
}


export const metadata = {
title: "Video Course",
};
Comment on lines +4 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance metadata with dynamic course information.

The current metadata title is static. Consider utilizing the route parameters to display the actual course title.

export const metadata = {
-  title: "Video Course",
+  title: async function generateTitle({ params }) {
+    const { slug } = params;
+    return `${slug.replace('-', ' ')} | Video Course`;
+  },
};

Committable suggestion was skipped due to low confidence.


export default async function Page() {
// Example of grabbing session in case it is needed
const session = await getServerAuthSession();

return <Content session={session} />;
}
Comment on lines +8 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Utilize route parameters and add error handling.

The page component doesn't use the URL parameters ([slug] and [id]) which are essential for fetching the correct course content.

Consider implementing proper error handling and course data fetching:

export default async function Page({
+ params,
+ }: {
+  params: { slug: string; id: string };
}) {
  const session = await getServerAuthSession();
+ 
+  // Fetch course data using params.slug and params.id
+  const course = await getCourse(params.slug, params.id);
+  
+  if (!course) {
+    notFound();
+  }

-  return <Content session={session} />;
+  return <Content session={session} course={course} />;
}

Committable suggestion was skipped due to low confidence.

118 changes: 118 additions & 0 deletions app/(app)/courses/mock.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// Mock video source (this would typically be a path to a video file)
const mockVideoSrc = "https://youtu.be/Ln4KSN0rchI?t=12";

// Mock content list
const mockContentList = [
{
id: 1,
title: "Introduction to React",
longDescription: "In this video, we will explore the fundamentals of React, including its component-based architecture, virtual DOM, and how to get started with your first React application. This tutorial guides you through the installation and configuration of essential tools like Node.js, npm, and create-react-app to set up your React development environment.",
description: "Learn the basics of React and its core concepts",
watched: false,
publishedDate: "2023-01-15",
author: "John Doe",
imageLink: "https://example.com/images/react-introduction.jpg"

},
{
id: 2,
title: "Setting Up Your Development Environment",
longDescription: "This tutorial guides you through the installation and configuration of essential tools like Node.js, npm, and create-react-app to set up your React development environment.",
description: "Install and configure the necessary tools for React development",
watched: true,
publishedDate: "2023-01-22",
author: "Jane Smith"
},
{
id: 3,
title: "Components and Props",
longDescription: "Dive deep into React's core concepts of components and props. Learn how to create reusable components and pass data between them using props.",
description: "Understand the building blocks of React applications",
watched: true,
publishedDate: "2023-01-29",
author: "John Doe"
},
{
id: 4,
title: "State and Lifecycle",
longDescription: "Explore how to manage component state and lifecycle methods to build dynamic and interactive applications with React.",
description: "Manage component state and lifecycle methods",
watched: true,
publishedDate: "2023-02-05",
author: "Jane Smith"
},
{
id: 5,
title: "Handling Events",
longDescription: "Learn how to handle user interactions in React, including events like clicks, form submissions, and more.",
description: "Learn how to handle user interactions in React",
watched: true,
publishedDate: "2023-02-12",
author: "John Doe"
},
{
id: 6,
title: "Conditional Rendering",
longDescription: "Discover how to display different UI elements based on specific conditions in your React applications.",
description: "Display different UI based on conditions",
watched: false,
publishedDate: "2023-02-19",
author: "Jane Smith"
},
{
id: 7,
title: "Lists and Keys",
longDescription: "Learn the best practices for rendering lists of components in React and how to efficiently manage them with unique keys.",
description: "Render multiple components efficiently",
watched: false,
publishedDate: "2023-02-26",
author: "John Doe"
},
{
id: 8,
title: "Forms in React",
longDescription: "This video covers how to create and manage forms in React applications, including controlled and uncontrolled components.",
description: "Create and handle form inputs in React applications",
watched: false,
publishedDate: "2023-03-05",
author: "Jane Smith"
},
{
id: 9,
title: "Hooks: useState and useEffect",
longDescription: "An introduction to React Hooks, focusing on useState and useEffect to manage state and side effects in functional components.",
description: "Understand and use basic React Hooks",
watched: false,
publishedDate: "2023-03-12",
author: "John Doe"
},
{
id: 10,
title: "Custom Hooks",
longDescription: "Learn how to create custom hooks in React to encapsulate and reuse logic across components.",
description: "Create reusable logic with custom Hooks",
watched: false,
publishedDate: "2023-03-19",
author: "Jane Smith"
},
{
id: 11,
title: "Context API",
longDescription: "Explore the Context API to manage global state in your React applications without the need for prop drilling.",
description: "Manage global state in your React application",
watched: false,
publishedDate: "2023-03-26",
author: "John Doe"
},
{
id: 12,
title: "React Router",
longDescription: "Learn how to implement navigation in your single-page applications using React Router, including dynamic routing and nested routes.",
description: "Implement navigation in your single-page application",
watched: false,
publishedDate: "2023-04-02",
author: "Jane Smith"
}
];

export { mockVideoSrc, mockContentList };
1 change: 1 addition & 0 deletions utils/flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { posthog } from "posthog-js";

export const FEATURE_FLAGS = {
FEATURE_FLAG_TEST: "feature-flag-test",
COURSE_VIDEO: "course-video",
JOBS: "jobs",
// Add more feature flags as needed
} as const;
Expand Down
Loading