feat: #93 구독모달 개발 #94
Conversation
WalkthroughThis pull request implements a new subscription feature on the HomePage, integrating a modal interface for user subscriptions. It introduces a dedicated Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant H as HomePage
participant M as Modal Hook
participant SM as SubscriptionModalContent
participant API as Subscription API
U->>H: Click "Subscribe" button
H->>M: openSubscriptionModal()
M->>SM: Open Subscription Modal with content
SM->>API: sendSubscribeEmail({ email })
API-->>SM: API Response
SM-->>U: Show subscription result
sequenceDiagram
participant U as User
participant S as Signup Page
participant API1 as SendVerifyEmail API
participant API2 as VerifyEmail API
U->>S: Enter email & request verification
S->>API1: sendVerifyEmail({ email, EMAIL_PURPOSE: SIGNUP })
API1-->>S: Email sent confirmation
U->>S: Input verification code
S->>API2: verifyEmail({ email, randomNumber, EMAIL_PURPOSE: SIGNUP })
API2-->>S: Verification result
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 2
🧹 Nitpick comments (9)
src/types/modal.ts (1)
3-3: LGTM! Consider a more specific property name.The optional boolean property is well-typed and follows TypeScript conventions. However, consider renaming it to be more specific about which button's visibility it controls (e.g.,
isActionButtonVisibleorisPrimaryButtonVisible).src/utils/common.ts (1)
6-9: Add JSDoc documentation for EMAIL_PURPOSE constant.The constant is well-defined with appropriate const assertion. Consider adding JSDoc documentation to explain its purpose and usage, similar to the existing comment above.
+/** + * Email purpose constants used for different email verification flows + */ const EMAIL_PURPOSE = { SIGNUP: 'SIGNUP', SUBSCRIBE: 'SUBSCRIBE', } as constsrc/hooks/api/member/useApiSendVerifyEmail.ts (1)
3-6: Consider using an enum or union type for emailPurpose.Using a string type for
emailPurposecould lead to typos or invalid values. Consider using an enum or union type to restrict the possible values.+export type EmailPurpose = 'SIGNUP' | 'SUBSCRIPTION'; + interface sendVerifyEmailProps { email: string; - emailPurpose: string; + emailPurpose: EmailPurpose; }src/hooks/api/member/useApiVerifyEmail.ts (1)
3-7: Maintain type consistency with useApiSendVerifyEmail.For consistency, use the same type for
emailPurposeas inuseApiSendVerifyEmail.+import { EmailPurpose } from './useApiSendVerifyEmail'; + interface verifyEmailProps { email: string; randomNumber: string; - emailPurpose: string; + emailPurpose: EmailPurpose; }src/components/common/modal/Modal.tsx (2)
12-12: Consider internationalizing UI strings.The UI strings ("취소", "확인") and comments are in Korean. Consider using an internationalization solution for better maintainability and future localization support.
- 취소 + {t('common.cancel')} - <Button onClick={modalDataState.callBack}>확인</Button> + <Button onClick={modalDataState.callBack}>{t('common.confirm')}</Button>Also applies to: 113-113, 115-115
93-105: Consider using CSS-in-JS theme variables.The margin and gap values could be moved to theme variables for better consistency across the application.
<div css={css` display:${modalDataState.isVisibleBtn ? 'flex' : 'none'}; align-items: center; - column-gap: 1.2rem; + column-gap: ${theme.spacing.md}; width: 100%; position: relative; margin-top: auto; div { width: 100%; } `} >src/components/common/modal/SubscriptionModalContent.tsx (2)
18-21: Consider using an enum or constant for subscription categories.The categories object could be defined as a constant or enum to maintain consistency and reusability across the application.
- const [categories, setCategories] = useState({ - bitcoinPrediction: false, - latestNews: false, - }); +const SUBSCRIPTION_CATEGORIES = { + BITCOIN_PREDICTION: 'bitcoinPrediction', + LATEST_NEWS: 'latestNews', +} as const; + + const [categories, setCategories] = useState({ + [SUBSCRIPTION_CATEGORIES.BITCOIN_PREDICTION]: false, + [SUBSCRIPTION_CATEGORIES.LATEST_NEWS]: false, + });
263-290: Improve button accessibility and styling consistency.The buttons should have consistent styling and proper accessibility attributes.
<Button css={css` width: 8.4rem !important; height: 4.5rem; ${DESIGN_SYSTEM_COLOR.GRAY_50} font-size: 1.6rem; font-weight: normal; left: 2rem; bottom: 2rem; `} + aria-label="취소" onClick={close} > 취소 </Button> <Button css={css` width: 8.4rem !important; height: 4.5rem; ${DESIGN_SYSTEM_COLOR.GRAY_50} font-size: 1.6rem; font-weight: normal; `} + aria-label="구독" + disabled={!verifyEmailNumCheck} state={Boolean(verifyEmailNumCheck)} onClick={handleCompleteSubscription} > 구독 </Button>src/app/home/index.tsx (1)
226-239: Consider adding loading state to the subscription button.The button should indicate when a subscription request is in progress.
+ const [isSubscribing, setIsSubscribing] = useState(false); + + const handleSubscriptionClick = async () => { + setIsSubscribing(true); + try { + await openSubscriptionModal(); + } finally { + setIsSubscribing(false); + } + }; <Button css={css` width: 11.4rem !important; height: 4.5rem; ${DESIGN_SYSTEM_COLOR.GRAY_50} font-size: 1.6rem; font-weight: normal; `} - onClick={openSubscriptionModal} + onClick={handleSubscriptionClick} + disabled={isSubscribing} > - 구독하기 + {isSubscribing ? '처리 중...' : '구독하기'} </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/app/home/index.tsx(3 hunks)src/app/signup/index.tsx(3 hunks)src/components/common/modal/Modal.tsx(2 hunks)src/components/common/modal/SubscriptionModalContent.tsx(1 hunks)src/hooks/api/member/useApiSendVerifyEmail.ts(1 hunks)src/hooks/api/member/useApiVerifyEmail.ts(1 hunks)src/hooks/api/subscribe/useApiSendSubscribe.ts(1 hunks)src/hooks/useModal.ts(1 hunks)src/store/modal/modalAtom.ts(1 hunks)src/types/modal.ts(1 hunks)src/utils/common.ts(1 hunks)
🔇 Additional comments (7)
src/store/modal/modalAtom.ts (1)
7-7: LGTM!The default value of true maintains backward compatibility for existing modals. The implementation follows Jotai atom patterns correctly.
src/hooks/api/member/useApiSendVerifyEmail.ts (1)
16-16: Empty hook implementation.The
useApiSendVerifyEmailhook is currently empty. Consider implementing proper hook functionality or removing it if not needed.src/hooks/api/member/useApiVerifyEmail.ts (1)
18-18: Empty hook implementation.The
useApiVerifyEmailhook is currently empty. Consider implementing proper hook functionality or removing it if not needed.src/hooks/useModal.ts (1)
14-25: LGTM! Clean implementation of modal visibility control.The implementation is well-structured with:
- Good use of TypeScript's Omit utility type
- Proper default value handling using nullish coalescing
- Clean callback implementation with useCallback
src/components/common/modal/Modal.tsx (1)
56-57: LGTM! Improved modal layout flexibility.The changes to
max-heightandrow-gapimprove the modal's adaptability to different content sizes.Also applies to: 64-64
src/app/signup/index.tsx (1)
17-17: LGTM!The changes correctly integrate the
EMAIL_PURPOSEconstant for email verification in the signup flow.Also applies to: 86-87, 109-110
src/app/home/index.tsx (1)
115-124: LGTM!The subscription modal opening logic is well-documented and correctly implemented.
| interface sendSubscribeEmail { | ||
| email: string; | ||
| } | ||
| export const sendSubscribeEmail = ({ email }: sendSubscribeEmail) => { | ||
| const res = api.post(`/user-service/api/v1/subscribers/subscribe`, { | ||
| email: email, | ||
| }); | ||
|
|
||
| return res; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed for the API hook implementation.
- Interface name should be PascalCase
- Add error handling
- Define response type
- Remove redundant property shorthand
- Add return type annotation
-interface sendSubscribeEmail {
+interface SendSubscribeEmailParams {
email: string;
}
-export const sendSubscribeEmail = ({ email }: sendSubscribeEmail) => {
+interface SubscribeResponse {
+ success: boolean;
+ message: string;
+}
+
+export const sendSubscribeEmail = async ({
+ email
+}: SendSubscribeEmailParams): Promise<SubscribeResponse> => {
+ try {
const res = api.post(`/user-service/api/v1/subscribers/subscribe`, {
- email: email,
+ email,
});
return res;
+ } catch (error) {
+ throw new Error('Failed to send subscription email');
+ }
};Committable suggestion skipped: line range outside the PR's diff.
| const handleCompleteSubscription = async () => { | ||
| if (!verifyEmailNumCheck) { | ||
| alert("이메일 인증을 완료해주세요."); | ||
| return; | ||
| } | ||
|
|
||
| const selectedCategories = Object.keys(categories).filter( | ||
| (key) => categories[key as keyof typeof categories] | ||
| ); | ||
|
|
||
| if (selectedCategories.length === 0) { | ||
| alert("최소 하나 이상의 구독 카테고리를 선택해주세요."); | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| await sendSubscribeEmail({ email }); | ||
| alert("구독이 완료되었습니다. 매일 자정에 구독 정보를 보내드립니다."); | ||
| close(); // 모달 창 닫기 | ||
| } catch (error) { | ||
| alert("구독 요청에 실패했습니다. 다시 시도해주세요."); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the subscription request.
The subscription request should handle specific error cases and provide appropriate feedback to users.
const handleCompleteSubscription = async () => {
if (!verifyEmailNumCheck) {
alert("이메일 인증을 완료해주세요.");
return;
}
const selectedCategories = Object.keys(categories).filter(
(key) => categories[key as keyof typeof categories]
);
if (selectedCategories.length === 0) {
alert("최소 하나 이상의 구독 카테고리를 선택해주세요.");
return;
}
try {
await sendSubscribeEmail({ email });
alert("구독이 완료되었습니다. 매일 자정에 구독 정보를 보내드립니다.");
close();
} catch (error) {
- alert("구독 요청에 실패했습니다. 다시 시도해주세요.");
+ if (error instanceof Error) {
+ if (error.message.includes('already subscribed')) {
+ alert("이미 구독 중인 이메일입니다.");
+ } else if (error.message.includes('invalid email')) {
+ alert("유효하지 않은 이메일입니다.");
+ } else {
+ alert("구독 요청에 실패했습니다. 다시 시도해주세요.");
+ }
+ }
}
};📝 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.
| const handleCompleteSubscription = async () => { | |
| if (!verifyEmailNumCheck) { | |
| alert("이메일 인증을 완료해주세요."); | |
| return; | |
| } | |
| const selectedCategories = Object.keys(categories).filter( | |
| (key) => categories[key as keyof typeof categories] | |
| ); | |
| if (selectedCategories.length === 0) { | |
| alert("최소 하나 이상의 구독 카테고리를 선택해주세요."); | |
| return; | |
| } | |
| try { | |
| await sendSubscribeEmail({ email }); | |
| alert("구독이 완료되었습니다. 매일 자정에 구독 정보를 보내드립니다."); | |
| close(); // 모달 창 닫기 | |
| } catch (error) { | |
| alert("구독 요청에 실패했습니다. 다시 시도해주세요."); | |
| } | |
| }; | |
| const handleCompleteSubscription = async () => { | |
| if (!verifyEmailNumCheck) { | |
| alert("이메일 인증을 완료해주세요."); | |
| return; | |
| } | |
| const selectedCategories = Object.keys(categories).filter( | |
| (key) => categories[key as keyof typeof categories] | |
| ); | |
| if (selectedCategories.length === 0) { | |
| alert("최소 하나 이상의 구독 카테고리를 선택해주세요."); | |
| return; | |
| } | |
| try { | |
| await sendSubscribeEmail({ email }); | |
| alert("구독이 완료되었습니다. 매일 자정에 구독 정보를 보내드립니다."); | |
| close(); // 모달 창 닫기 | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| if (error.message.includes('already subscribed')) { | |
| alert("이미 구독 중인 이메일입니다."); | |
| } else if (error.message.includes('invalid email')) { | |
| alert("유효하지 않은 이메일입니다."); | |
| } else { | |
| alert("구독 요청에 실패했습니다. 다시 시도해주세요."); | |
| } | |
| } | |
| } | |
| }; |
klmhyeonwoo
added
feature
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/app/signup/index.tsx (1)
104-117: Improve error handling.The error handling could be enhanced:
- Remove console.log in production
- Consider providing more specific error messages based on error types
.catch((err) => { - console.log(err); alert( - "인증번호 입력 중 오류가 발생했습니다, 관리자에게 문의해주세요!", + err.response?.data?.message || + "인증번호 입력 중 오류가 발생했습니다, 관리자에게 문의해주세요!" ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/app/signup/index.tsx(3 hunks)src/components/common/modal/SubscriptionModalContent.tsx(1 hunks)src/hooks/api/member/useApiSendVerifyEmail.ts(1 hunks)src/hooks/api/member/useApiVerifyEmail.ts(1 hunks)src/utils/regex.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/utils/regex.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/hooks/api/member/useApiVerifyEmail.ts
- src/hooks/api/member/useApiSendVerifyEmail.ts
- src/components/common/modal/SubscriptionModalContent.tsx
🔇 Additional comments (2)
src/app/signup/index.tsx (2)
17-18: LGTM! Good refactoring of constants.Moving EMAIL_REGEX to a shared location and introducing EMAIL_PURPOSE improves code reusability and standardization.
84-84: LGTM! Good addition of email purpose context.The addition of emailPurpose parameter helps distinguish between signup and subscription verification flows.
🙋 Summary (요약)
🤔 Describe your Change (변경사항)
🔗 Issue number and link (참고)
Summary by CodeRabbit
New Features
Style