Update webinar changes #289
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nxtCoder19
requested review from
karthik1729 and
abdheshnayak
as code owners
Reviewer's Guide by SourceryThis pull request updates the webinar application, removes cluster-related code from the console application, and makes various other changes across multiple files. The main focus is on updating the webinar application to handle user authentication and session management, while also removing dependencies on cluster information in the console application. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nxtCoder19 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
Overall Comments:
- Please provide a detailed description of the changes and their purpose. The lack of context makes it difficult to understand the motivation behind these modifications.
- Consider adopting a consistent approach to removing or deprecating code. Currently, some parts are fully removed while others are commented out, which could lead to confusion.
- Could you explain the testing strategy for these changes? Given the scope of the modifications, it's important to ensure that all affected functionality has been thoroughly tested.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 2 blocking issues
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| </div> | ||
| <div className="bodyMd-medium text-text-soft"> | ||
| Join webinar and experience the power of Kloudlite | ||
| export default async function Home() { |
There was a problem hiding this comment.
issue (complexity): Consider extracting authentication logic into a custom hook.
The Home component has become more complex with the addition of authentication and API call logic. To improve readability and maintainability, consider extracting this logic into a custom hook. This will separate concerns and make the component easier to understand while retaining all new functionality.
Here's an example of how you could refactor this:
// useAuth.ts
import { useState, useEffect } from 'react';
import axios from 'axios';
import { cookies } from 'next/headers';
export function useAuth() {
const [userData, setUserData] = useState(null);
const [isLoading, setIsLoading] = useState(true);
useEffect(() => {
async function authenticate() {
const cookie = cookies().get("hotspot-session");
const callbackUrl = "https://auth-piyush.dev.kloudlite.io";
const redirectUrl = "https://auth.dev.kloudlite.io/login?callback=" + callbackUrl;
try {
const res = await axios({
url: `${process.env.NEXT_PUBLIC_AUTH_URL}/api` || 'https://auth.kloudlite.io/api',
method: 'post',
data: {
method: 'whoAmI',
args: [{}],
},
headers: {
'Content-Type': 'application/json; charset=utf-8',
connection: 'keep-alive',
cookie: 'hotspot-session=' + cookie?.value + ';',
},
});
setUserData(res.data.data);
} catch (e) {
window.location.href = redirectUrl;
} finally {
setIsLoading(false);
}
}
authenticate();
}, []);
return { userData, isLoading };
}
// Home.tsx
import { useAuth } from './useAuth';
import { JoinWebinar } from './components/join-webinar';
export default function Home() {
const { userData, isLoading } = useAuth();
if (isLoading) {
return <div>Loading...</div>;
}
if (!userData) {
return null; // or a loading state, this will be brief before redirect
}
return (
<main className='flex flex-col h-full'>
<div className='flex flex-1 flex-col md:items-center self-stretch justify-center px-3xl py-5xl md:py-9xl'>
<div className='flex flex-col gap-3xl md:w-[500px] px-3xl py-5xl md:px-9xl'>
<div className='flex flex-col items-stretch"'>
<div className="flex flex-col gap-lg items-center pb-6xl text-center">
<div className={cn('text-text-strong headingXl text-center')}>
Join Kloudlite webinar
</div>
<div className="bodyMd-medium text-text-soft">
Join webinar and experience the power of Kloudlite
</div>
</div>
<JoinWebinar userData={userData} />
{userData.email}
</div>
</div>
</div>
</main>
);
}This refactoring:
- Extracts authentication logic into a
useAuthhook. - Simplifies the Home component, focusing it on rendering.
- Handles loading and error states more explicitly.
- Improves reusability of the authentication logic.
Remember to adjust imports and types as necessary for your project structure.
| export default async function Home() { | ||
|
|
||
| const cookie = cookies().get("hotspot-session") | ||
| const callbackUrl = "https://auth-piyush.dev.kloudlite.io"; |
There was a problem hiding this comment.
🚨 issue (security): Hardcoded URL found in the code.
Consider using an environment variable to store URLs to avoid hardcoding them in the codebase.
|
|
||
| const cookie = cookies().get("hotspot-session") | ||
| const callbackUrl = "https://auth-piyush.dev.kloudlite.io"; | ||
| const redirectUrl = "https://auth.dev.kloudlite.io/login?callback=" + callbackUrl; |
There was a problem hiding this comment.
🚨 issue (security): Hardcoded URL found in the code.
Consider using an environment variable to store URLs to avoid hardcoding them in the codebase.
| cookie: 'hotspot-session=' + cookie?.value + ';', | ||
| }, | ||
| }); | ||
| const data = res.data.data; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const data = res.data.data; | |
| const {data} = res.data; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Enhance the webinar application by integrating user authentication, refactor the webinar and meeting pages for better data handling and loading management, update TypeScript configuration, and clean up the codebase by commenting out unused code.
New Features:
Enhancements:
Build:
Chores: