Newsletter subscription #186
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
WalkthroughThis pull request introduces enhancements to the newsletter subscription feature, modifies the modal component's visibility and styling, and adjusts padding in the HomeLayout. It also adds error handling for form submission and a new function to perform GraphQL mutations. Changes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Files selected (4)
- app/home-page.tsx (3)
- components/Home/Modal.tsx (1)
- components/Home/Newsletter.tsx (4)
- lib/subscribeUser.ts (1)
Files skipped from review due to trivial changes (1)
- components/Home/Modal.tsx
Review comments generated (12)
- Review: 8
- LGTM: 4
Tips
Chat with 
CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| user_name: "${name}" | ||
| user_email: "${email}" |
There was a problem hiding this comment.
The GraphQL mutation string is being constructed using template literals which can lead to potential security issues like GraphQL injection attacks. It's better to use variables in the mutation and pass the values separately.
- user_name: "${name}"
- user_email: "${email}"
+ user_name: $name
+ user_email: $emailAnd then in the mutate function, pass the variables like this:
await apolloClient.mutate({
mutation: gql`
mutation MyMutation($name: String!, $email: String!) {
insert_newsletter_subscriptions_one(
object: {
user_name: $name
user_email: $email
}
) {
user_name
user_email
}
}
`,
+ variables: {
+ name,
+ email,
+ },
});| import { SubscribeMessage } from "../../app/home-page"; | ||
|
|
||
| interface Props { | ||
| handleSubscribe: ( | ||
| e: FormEvent<HTMLFormElement>, | ||
| formData: NewsletterFormData | ||
| ) => boolean; | ||
| ) => Promise<SubscribeMessage>; |
There was a problem hiding this comment.
The change from a boolean return type to a Promise of SubscribeMessage in the handleSubscribe function is a good improvement. It allows for better error handling and more informative responses.
- ) => boolean;
+ ) => Promise<SubscribeMessage>;| const [errorMessage, setErrorMessage] = useState(""); | ||
|
|
||
| const onSubmit = async (e) => { | ||
| const { isSuccess, message } = await handleSubscribe(e, formData); | ||
| if (isSuccess) { | ||
| setFormData({ name: "", email: "" }); | ||
| setIsValid(true); | ||
| setErrorMessage(""); | ||
| } else { | ||
| setIsValid(false); | ||
| setErrorMessage(message); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The addition of an errorMessage state variable and its usage in the onSubmit function is a good practice. This allows for dynamic error messages based on the response from the handleSubscribe function, improving user experience.
+ const [errorMessage, setErrorMessage] = useState("");
+
+ const onSubmit = async (e) => {
+ const { isSuccess, message } = await handleSubscribe(e, formData);
+ if (isSuccess) {
+ setFormData({ name: "", email: "" });
+ setIsValid(true);
+ setErrorMessage("");
+ } else {
+ setIsValid(false);
+ setErrorMessage(message);
+ }
+ };| } | ||
| }} | ||
| > | ||
| <form className="flex flex-col md:flex-row" onSubmit={onSubmit}> |
There was a problem hiding this comment.
The refactoring of the form's onSubmit event handler to use the new onSubmit function is a good change. It simplifies the code and makes it easier to understand.
- onSubmit={(e) => {
- if (handleSubscribe(e, formData)) {
- setFormData({ name: "", email: "" });
- setIsValid(true);
- } else {
- setIsValid(false);
- }
- }}
+ onSubmit={onSubmit}| <div className="text-lg text-red-400 mt-4"> | ||
| <p className="font-bold mr-20 mt-12">{errorMessage}</p> |
There was a problem hiding this comment.
The change to display the errorMessage when the form is not valid is a good improvement. It provides more specific feedback to the user about what went wrong.
- <p className="font-bold mr-20 mt-12">
- ERROR: Name or Email Cannot be Empty
- </p>
+ <p className="font-bold mr-20 mt-12">{errorMessage}</p>|
|
||
| async function handleSubscribe( | ||
| e: FormEvent<HTMLFormElement>, | ||
| formData: NewsletterFormData | ||
| ): boolean { | ||
| { name, email }: NewsletterFormData | ||
| ): Promise<SubscribeMessage> { | ||
| e.preventDefault(); | ||
| if (formData.name && formData.email) { | ||
| // todo call newsletter subscribe API | ||
| setModalVisible(true); | ||
| return true; | ||
| } else return false; | ||
| if (name && email) { | ||
| try { | ||
| await subscribeUser(name, email); | ||
|
|
||
| setModalVisible(true); | ||
| return { | ||
| isSuccess: true, | ||
| message: "", | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| isSuccess: false, | ||
| message: "ERROR: Email already exists", | ||
| }; | ||
| } | ||
| } else | ||
| return { | ||
| isSuccess: false, | ||
| message: "ERROR: Name or Email Cannot be Empty", | ||
| }; |
There was a problem hiding this comment.
The handleSubscribe function has been updated to use async/await syntax and includes error handling, which is good. However, the error message "ERROR: Email already exists" assumes that any error thrown by subscribeUser is due to a duplicate email. This may not always be the case. It would be better to check the error message returned from the server and display that instead. Also, consider using a try-catch-finally block to ensure setModalVisible(true) is called regardless of whether an error occurs.
- } catch (error) {
- return {
- isSuccess: false,
- message: "ERROR: Email already exists",
- };
- }
+ } catch (error) {
+ return {
+ isSuccess: false,
+ message: error.message || "An error occurred",
+ };
+ } finally {
+ setModalVisible(true);
+ }
There was a problem hiding this comment.
Files selected (3)
- app/home-page.tsx (2)
- components/Home/Modal.tsx (2)
- layouts/HomeLayout.tsx (1)
Files skipped from review due to trivial changes (1)
- components/Home/Modal.tsx
Review comments generated (6)
- Review: 3
- LGTM: 3
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| @@ -6,7 +6,7 @@ const HomeLayout = ({ children }: { children: ReactNode }) => { | |||
| return ( | |||
| <div className="dark:bg-dark-bg min-h-screen flex flex-col"> | |||
| <IndexHeader /> | |||
| <div className="pt-24 lg:pt-[90px] flex-1">{children}</div> | |||
| <div className="pt-[84px] lg:pt-[90px] flex-1">{children}</div> | |||
There was a problem hiding this comment.
The change from pt-24 to pt-[84px] seems to be a significant increase in padding-top. Ensure that this change doesn't disrupt the layout of the page or make it inconsistent with other pages. If the intention is to provide more space, consider if there's a more responsive way to do so.
- <div className="pt-[84px] lg:pt-[90px] flex-1">{children}</div>
+ <div className="pt-24 lg:pt-[90px] flex-1">{children}</div>| async function handleSubscribe( | ||
| e: FormEvent<HTMLFormElement>, | ||
| formData: NewsletterFormData | ||
| ): boolean { | ||
| { name, email }: NewsletterFormData | ||
| ): Promise<SubscribeMessage> { | ||
| e.preventDefault(); | ||
| if (formData.name && formData.email) { | ||
| // todo call newsletter subscribe API | ||
| setModalVisible(true); | ||
| return true; | ||
| } else return false; | ||
| if (name && email) { | ||
| try { | ||
| await subscribeUser(name, email); | ||
|
|
||
| setModalVisible(true); | ||
| return { | ||
| isSuccess: true, | ||
| message: "", | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| isSuccess: false, | ||
| message: "ERROR: Email already exists", | ||
| }; | ||
| } | ||
| } else | ||
| return { | ||
| isSuccess: false, | ||
| message: "ERROR: Name or Email Cannot be Empty", | ||
| }; | ||
| } |
There was a problem hiding this comment.
The handleSubscribe function has been modified to be asynchronous and now returns a promise that resolves to a SubscribeMessage. The error handling introduced here is a good practice, but it's not ideal to hardcode error messages like "ERROR: Email already exists" or "ERROR: Name or Email Cannot be Empty". It would be better to handle these errors at a higher level or use an error handling utility. Here's a suggestion:
try {
await subscribeUser(name, email);
setModalVisible(true);
return {
isSuccess: true,
message: "",
};
} catch (error) {
+ let errorMessage = '';
+ if (error.message.includes('duplicate key value')) {
+ errorMessage = 'ERROR: Email already exists';
+ } else {
+ errorMessage = error.message;
+ }
return {
isSuccess: false,
- message: "ERROR: Email already exists",
+ message: errorMessage,
};
}This way, you can handle different types of errors more effectively.
app/home-page.tsx
Outdated
| return ( | ||
| <div className="font-inter min-h-screen dark:bg-dark-bg"> | ||
| <main> | ||
| <HomeLayout> | ||
| <Modal | ||
| modalVisible={modalVisible} | ||
| modalVisible={!modalVisible} | ||
| setModalVisible={setModalVisible} | ||
| /> | ||
| <div className="relative"> | ||
| <Banner /> | ||
| <div className="absolute top-[204px] z-0 w-full h-[460px]"> | ||
| <Image src="/main-background.png" alt="background image" fill /> | ||
| </div> | ||
| <Image | ||
| src="/flower.svg" | ||
| alt="flower" | ||
| width={365} | ||
| height={150} | ||
| className="absolute top-[54%] left-[50%] -translate-x-2/4" | ||
| /> | ||
| {theme === "light" && ( | ||
| <> | ||
| <div className="absolute top-[204px] z-0 w-full h-[460px]"> | ||
| <Image | ||
| src="/main-background.png" | ||
| alt="background image" | ||
| fill | ||
| /> | ||
| </div> | ||
| <Image | ||
| src="/flower.svg" | ||
| alt="flower" | ||
| width={365} | ||
| height={150} | ||
| className="absolute top-[54%] left-[50%] -translate-x-2/4" | ||
| /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
The rendering of certain elements is now conditional based on the theme value. This is a good approach for theming. However, the modal visibility condition seems to be inverted in line 71. If modalVisible is true, then !modalVisible will be false, which means the modal won't be visible when it should be. Here's a suggested fix:
- <Modal modalVisible={!modalVisible} setModalVisible={setModalVisible} />
+ <Modal modalVisible={modalVisible} setModalVisible={setModalVisible} />
There was a problem hiding this comment.
Files selected (1)
- app/home-page.tsx (3)
Review comments generated (4)
- Review: 3
- LGTM: 1
Tips
Chat with CodeRabbit Bot (@coderabbitai)
- Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
- Invite the bot into a review comment chain by tagging
@coderabbitaiin a reply.
Code suggestions
- The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
- You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.
Pausing incremental reviews
- Add
@coderabbitai: ignoreanywhere in the PR description to pause further reviews from the bot.
| import { useTheme } from "next-themes"; | ||
| import { subscribeUser } from "../lib/subscribeUser"; | ||
|
|
||
| export type SubscribeMessage = { isSuccess: boolean; message: string }; |
There was a problem hiding this comment.
The SubscribeMessage type is defined but it's not clear if the message field is used in any meaningful way. It seems to be set to an empty string on success and a hardcoded error message on failure. Consider using this field to pass more detailed error messages from the subscribeUser function, or remove it if it's not needed.
- export type SubscribeMessage = { isSuccess: boolean; message: string };
+ export type SubscribeMessage = { isSuccess: boolean; message?: string };| async function handleSubscribe( | ||
| e: FormEvent<HTMLFormElement>, | ||
| formData: NewsletterFormData | ||
| ): boolean { | ||
| { name, email }: NewsletterFormData | ||
| ): Promise<SubscribeMessage> { | ||
| e.preventDefault(); | ||
| if (formData.name && formData.email) { | ||
| // todo call newsletter subscribe API | ||
| setModalVisible(true); | ||
| return true; | ||
| } else return false; | ||
| if (name && email) { | ||
| try { | ||
| await subscribeUser(name, email); | ||
|
|
||
| setModalVisible(true); | ||
| return { | ||
| isSuccess: true, | ||
| message: "", | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| isSuccess: false, | ||
| message: "ERROR: Email already exists", | ||
| }; | ||
| } | ||
| } else | ||
| return { | ||
| isSuccess: false, | ||
| message: "ERROR: Name or Email Cannot be Empty", | ||
| }; |
There was a problem hiding this comment.
The handleSubscribe function has been modified to handle errors and return a SubscribeMessage. However, it only catches errors at a high level and assumes any error means that the email already exists. This could lead to misleading error messages if the error was due to something else (like a network error). Consider catching specific error types within the subscribeUser function and throwing custom Error objects with appropriate messages that can be caught and handled here.
Also, the function returns a SubscribeMessage object when the name or email fields are empty. This is not an asynchronous operation and doesn't need to be part of an async function. Consider moving this validation outside of handleSubscribe and before calling it.
- async function handleSubscribe(
- e: FormEvent<HTMLFormElement>,
- { name, email }: NewsletterFormData
- ): Promise<SubscribeMessage> {
- e.preventDefault();
- if (name && email) {
- try {
- await subscribeUser(name, email);
-
- setModalVisible(true);
- return {
- isSuccess: true,
- message: "",
- };
- } catch (error) {
- return {
- isSuccess: false,
- message: "ERROR: Email already exists",
- };
- }
- } else
- return {
- isSuccess: false,
- message: "ERROR: Name or Email Cannot be Empty",
- };
- }
+ function validateNewsletterFormData({ name, email }: NewsletterFormData): boolean {
+ if (!name || !email) {
+ // Show error message about empty fields
+ return false;
+ }
+ return true;
+ }
+
+ async function handleSubscribe(
+ e: FormEvent<HTMLFormElement>,
+ formData: NewsletterFormData
+ ): Promise<SubscribeMessage> {
+ e.preventDefault();
+
+ try {
+ await subscribeUser(formData.name, formData.email);
+ setModalVisible(true);
+ return {
+ isSuccess: true,
+ };
+ } catch (error) {
+ // Assume error.message is set appropriately in subscribeUser
+ return {
+ isSuccess: false,
+ message: error.message,
+ };
+ }
+ }| {theme === "light" && ( | ||
| <> | ||
| <div className="absolute top-[204px] z-0 w-full h-[460px]"> | ||
| <Image | ||
| src="/main-background.png" | ||
| alt="background image" | ||
| fill | ||
| /> | ||
| </div> | ||
| <Image | ||
| src="/flower.svg" | ||
| alt="flower" | ||
| width={365} | ||
| height={150} | ||
| className="absolute top-[54%] left-[50%] -translate-x-2/4" | ||
| /> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
The conditional rendering based on the theme value is a good practice for theme-based UI changes. However, it's not clear why the Image components are only rendered for the light theme. If they should also be rendered for other themes, consider moving them outside of the conditional block. If they're specific to the light theme, this is fine as is.
Summary by CodeRabbit
New Feature:
HomePagecomponent with improved error handling and conditional rendering based on theme.Newsletter.tsxto display error messages when form submission fails.subscribeUserinlib/subscribeUser.tsfor GraphQL mutation.Style:
HomeLayout.tsxfor better positioning of child components.Modalcomponent incomponents/Home/Modal.tsx.