[2단계 - 페이먼츠 모듈] 메리(이수화) 미션 제출합니다.#154
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
Walkthrough이번 변경 사항은 React 모달 컴포넌트와 커스텀 훅 라이브러리의 구조와 문서, 그리고 주요 구현 방식을 대대적으로 개편하는 내용입니다. 모달 컴포넌트는 기존 단일 파일 구조에서 다양한 하위 컴포넌트와 컨텍스트, 포커스 트랩 훅, 스타일 컴포넌트 등으로 분할되었으며, Storybook 스토리와 문서도 새롭게 작성되었습니다. 버튼과 인풋 컴포넌트 역시 스타일 및 타입 정의와 함께 새로 추가되었습니다. 커스텀 훅 라이브러리에서는 카드 번호 입력 훅이 브랜드별 동적 필드 분할, 유효성 검사, 에러 처리, 브랜드 감지 등으로 확장되었고, 타입과 상수, 테스트 코드도 이에 맞게 리팩터링되었습니다. 각 패키지의 README와 package.json도 새로운 네이밍 및 상세 설명으로 갱신되었습니다. CSS 파일들은 삭제되거나 변경되었으며, 전체적으로 코드와 문서가 구조화되고 확장성 있게 재구성되었습니다. Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 13
🔭 Outside diff range comments (2)
hooks/src/lib/useCardPassword/useCardPassword.ts (1)
29-31: 🛠️ Refactor suggestion유효성 검사 로직 개선 제안
isCardPasswordValid함수에서 몇 가지 개선할 점이 있습니다:
cardPassword !== null검사는 불필요합니다. useState에서 초기값이 문자열이므로 null이 될 수 없습니다.cardPassword.toString().length는 cardPassword가 이미 문자열이므로cardPassword.length로 간소화할 수 있습니다.const isCardPasswordValid = () => { - return cardPassword !== null && cardPassword.toString().length === CARD_PASSWORD.maxLength && !cardPasswordError; + return cardPassword.length === CARD_PASSWORD.maxLength && !cardPasswordError; };이렇게 하면 코드가 더 간결해지고 불필요한 연산이 제거됩니다.
hooks/src/lib/useCardCVC/useCardCVC.ts (1)
25-27: 🛠️ Refactor suggestion유효성 검사 일관성 개선 제안
isCardCVCValid함수에서 옵셔널 체이닝(?.)을 사용하고 있지만, useState에서 초기값이 문자열이므로 null이나 undefined가 될 수 없습니다. 또한useCardPassword와 일관성을 유지하기 위해 에러 상태도 검사하는 것이 좋습니다.const isCardCVCValid = () => { - return cardCVC !== '' && cardCVC?.length === CARD_CVC.maxLength; + return cardCVC !== '' && cardCVC.length === CARD_CVC.maxLength && !cardCVCError; };이렇게 하면 불필요한 옵셔널 체이닝이 제거되고,
useCardPassword와 동일한 유효성 검사 패턴을 사용하게 됩니다.
🧹 Nitpick comments (25)
hooks/src/lib/useCardNumber/identifyCardBrand.ts (1)
3-16: 카드 브랜드 식별 함수 리팩토링 제안현재 if-else 체인을 사용하여 카드 브랜드를 식별하는 구현은 작동하지만, 확장성과 유지보수 관점에서 개선할 여지가 있습니다. CARD_RULES 객체의 속성을 활용하여 더 간결하고 확장 가능한 구현으로 리팩토링할 수 있습니다.
- const identifyCardBrand = (cardNumber: string) => { - if (CARD_RULES[CardBrandType.VISA].pattern.test(cardNumber)) { - return CardBrandType.VISA; - } else if (CARD_RULES[CardBrandType.MASTERCARD].pattern.test(cardNumber)) { - return CardBrandType.MASTERCARD; - } else if (CARD_RULES[CardBrandType.AMEX].pattern.test(cardNumber)) { - return CardBrandType.AMEX; - } else if (CARD_RULES[CardBrandType.DINERS].pattern.test(cardNumber)) { - return CardBrandType.DINERS; - } else if (CARD_RULES[CardBrandType.UNIONPAY].pattern.test(cardNumber)) { - return CardBrandType.UNIONPAY; - } - return CardBrandType.UNKNOWN; - }; + const identifyCardBrand = (cardNumber: string) => { + const cardBrand = Object.entries(CARD_RULES) + .find(([, rules]) => rules.pattern.test(cardNumber))?.[0]; + + return cardBrand || CardBrandType.UNKNOWN; + };이 방식을 사용하면 새로운 카드 유형이 추가되더라도 함수를 변경할 필요 없이
cardRules.ts에만 새 규칙을 추가하면 됩니다.hooks/src/lib/useCardNumber/cardRules.ts (1)
1-8: 타입 정의 개선 제안현재
CardBrandType은 객체로 정의되어 있지만, TypeScript의 강력한 타입 시스템을 활용할 수 있는 더 적합한 방법들이 있습니다. 이는 컴파일 시간에 더 많은 오류를 잡을 수 있게 해줍니다.- export const CardBrandType = { - UNKNOWN: 'UNKNOWN', - VISA: 'VISA', - MASTERCARD: 'MASTERCARD', - AMEX: 'AMEX', - DINERS: 'DINERS', - UNIONPAY: 'UNIONPAY', - }; + export enum CardBrandType { + UNKNOWN = 'UNKNOWN', + VISA = 'VISA', + MASTERCARD = 'MASTERCARD', + AMEX = 'AMEX', + DINERS = 'DINERS', + UNIONPAY = 'UNIONPAY', + }또는 문자열 리터럴 유니온 타입을 사용할 수도 있습니다:
export type CardBrandType = | 'UNKNOWN' | 'VISA' | 'MASTERCARD' | 'AMEX' | 'DINERS' | 'UNIONPAY'; export const CardBrand: Record<CardBrandType, CardBrandType> = { UNKNOWN: 'UNKNOWN', VISA: 'VISA', MASTERCARD: 'MASTERCARD', AMEX: 'AMEX', DINERS: 'DINERS', UNIONPAY: 'UNIONPAY', };이런 방식을 사용하면 타입 안전성이 향상되고 자동 완성 기능도 더 잘 작동합니다.
hooks/src/lib/types/cardTypes.ts (1)
3-4: 타입 개선이 잘 이루어졌습니다.타입 정의가 카드 번호를 단일 문자열로 처리하고 오류를 배열로 관리하도록 변경된 것은 카드 브랜드에 따라 동적으로 필드 길이를 변경하는 구현에 적합합니다. 특히
CardNumberError를 배열로 변경함으로써 각 세그먼트별 오류 메시지를 더 유연하게 처리할 수 있게 되었습니다.이러한 타입 변경은 더 유연한 구현을 가능하게 하지만, 타입스크립트의 장점을 더 활용하려면 브랜드별 타입도 고려해볼 수 있습니다. 예를 들어
CardBrand타입을 명시적으로 정의하면 타입 안전성을 더 높일 수 있습니다.hooks/src/lib/useCardNumber/useCardNumber.test.ts (1)
95-119: 브랜드 변경 시 필드 동적 조정 테스트가 잘 구현되었습니다.카드 번호 입력에 따라 브랜드가 변경될 때 필드 구성이 동적으로 조정되는지 확인하는 테스트가 포괄적입니다. UNKNOWN에서 VISA로, 그리고 VISA에서 AMEX로의 전환 시 필드 수와 길이 변화를 적절히 검증하고 있습니다.
테스트의 주석은 테스트 의도를 더 명확히 하는 데 도움이 됩니다. 하지만 일관성을 위해 모든 주석 끝에 마침표를 추가하는 것이 좋을 것 같습니다.
hooks/README.md (3)
33-83: 기본 사용법 예제가 유용합니다.훅의 사용 방법을 보여주는 예제 컴포넌트가 잘 구성되어 있습니다. 반환된 값들을 활용하여 UI를 구성하는 방법이 명확하게 설명되어 있습니다.
React 컴포넌트 예제에서
key속성을 배열 인덱스로 사용하는 것은 일반적이지만, 항목이 재정렬될 경우 문제가 발생할 수 있습니다. 이 경우에는 큰 문제가 아니지만, 더 복잡한 시나리오에서는 고유 식별자를 사용하는 것이 좋습니다. 이에 대한 설명을 README에 추가하시겠습니까?
116-127: 카드 브랜드 감지 규칙이 잘 설명되어 있습니다.지원되는 카드 브랜드와 그 식별 규칙이 명확하게 설명되어 있습니다. 사용자가 어떤 카드 브랜드가 자동으로 감지되는지 이해하는 데 도움이 됩니다.
Diners 카드에 대한 설명이 있지만, 앞서 테스트 코드에서는 AMEX와 Mastercard만 테스트되고 있습니다. 일관성을 위해 모든 지원되는 카드 브랜드에 대한 테스트를 추가하는 것이 좋을 것 같습니다.
130-197: 고급 사용 예제가 포괄적입니다.다양한 기능을 활용한 고급 사용 예제가 잘 제공되어 있습니다. 포커스 상태 관리, 폼 제출 처리, 스타일링 등 실제 애플리케이션에서 활용할 수 있는 방법이 상세히 설명되어 있습니다.
고급 예제에서
isFocused상태의 초기화와 관리가 하드코딩된 배열 길이(Array(4))로 되어 있습니다. 이를requiredFields값을 사용하도록 변경하면 카드 브랜드에 따라 동적으로 대응할 수 있습니다:- const [isFocused, setIsFocused] = useState(Array(4).fill(false)); + const [isFocused, setIsFocused] = useState(Array(requiredFields).fill(false)); const handleFocus = (idx) => { - const newFocused = Array(4).fill(false); + const newFocused = Array(requiredFields).fill(false); newFocused[idx] = true; setIsFocused(newFocused); };hooks/src/App.tsx (2)
30-48: 카드 정보 표시가 잘 구현되었습니다.카드 번호, 타입, 포맷팅된 카드 번호, 오류 메시지, 유효성 상태 등 중요 정보를 명확하게 표시하고 있습니다. 특히
some과find메서드를 사용하여 오류 메시지를 효율적으로 처리한 점이 좋습니다.오류 메시지 표시 로직에서
find는 첫 번째 오류만 반환합니다. 만약 여러 필드에 오류가 있을 경우 모든 오류를 표시하는 것이 사용자 경험을 향상시킬 수 있습니다:- {cardNumberError.some((error) => error) && ( - <div style={{ color: 'red' }}> - <strong>오류:</strong> {cardNumberError.find((error) => error)} - </div> - )} + {cardNumberError.some((error) => error) && ( + <div style={{ color: 'red' }}> + <strong>오류:</strong> + <ul> + {cardNumberError.map((error, idx) => + error ? <li key={idx}>{error}</li> : null + )} + </ul> + </div> + )}
1-53: 코드 구조와 접근 방식에 관한 종합 의견이 컴포넌트는
useCardNumber훅의 기능을 효과적으로 보여주는 좋은 예시입니다. 카드 번호 입력, 유효성 검사, 브랜드 감지 등의 기능이 잘 통합되어 있습니다.개발자가 훅의 기능을 점진적으로 확장해 나가는 과정이 코드에서 엿보입니다. 초기에는 단순한 카드 번호 처리에서 시작하여 카드 브랜드 감지, 동적 필드 조정 등으로 기능을 확장한 것으로 보입니다. 이는 React 생태계의 컴포넌트 기반 접근 방식과 훅의 재사용성을 잘 활용한 사례입니다.
향후 확장을 고려한다면, 테마 지원이나 다국어 오류 메시지와 같은 기능을 추가할 수 있을 것입니다. 또한 접근성(a11y) 개선을 위한 ARIA 속성을 더 추가하는 것도 고려해 볼 수 있습니다.
components/src/lib/components/Button/Button.types.ts (1)
1-2: 타입 정의가 명확하고 간결합니다.타입 정의가 명확하게 되어 있어 좋습니다. 하지만 향후 확장성을 고려한다면 각 타입에 대한 간단한 JSDoc 주석을 추가하는 것이 도움이 될 수 있습니다. 예를 들어:
/** 버튼 변형 타입 - 시각적 스타일을 결정합니다 */ export type ButtonVariant = 'primary' | 'secondary'; /** 버튼 크기 타입 - 버튼의 크기를 결정합니다 */ export type ButtonSize = 'small' | 'medium' | 'large';이러한 주석은 다른 개발자들이 컴포넌트를 사용할 때 각 타입의 목적을 더 쉽게 이해할 수 있도록 도와줍니다.
components/src/lib/components/Input/Input.types.ts (1)
1-5: 인터페이스 확장이 잘 구성되어 있습니다.기본 HTML 인풋 속성을 확장하는 방식으로 타입을 정의한 것이 좋습니다. 다만, 각 속성에 대한 설명을 JSDoc 주석으로 추가하면 API 문서화에 도움이 될 것입니다.
export interface InputProps extends React.InputHTMLAttributes<HTMLInputElement> { /** 입력 필드의 너비를 100%로 설정합니다 */ fullWidth?: boolean; /** 오류 상태를 표시합니다 */ error?: boolean; /** 사용자 정의 클래스 이름을 적용합니다 */ className?: string; }또한 error 상태가 있다면, 오류 메시지를 표시하기 위한
errorMessage속성도 고려해 볼 수 있을 것입니다.components/src/lib/components/Modal/Modal.types.ts (1)
3-10: 타입 구조가 잘 분리되어 있으나 추가 문서화가 필요합니다.BaseProps와 ModalProps로 타입을 분리한 구조가 확장성 측면에서 좋습니다. 하지만 각 타입의 목적과 사용법에 대한 문서화가 있으면 더 명확할 것입니다.
/** * 모든 컴포넌트의 기본 속성 */ export type BaseProps = { children?: React.ReactNode; className?: string; }; /** * 모달 컴포넌트의 속성 * BaseProps를 확장하여 size 속성을 추가합니다. */ export type ModalProps = BaseProps & { size?: ModalSize; };또한 사용자 정의 모달 위치 지정을 위한
position속성도 고려해볼만 합니다. 모달의 위치(중앙, 상단, 하단 등)를 설정할 수 있으면 더 유연한 모달을 구현할 수 있습니다.components/src/lib/components/Button/Button.styles.ts (2)
4-70: 스타일 시스템 개선 제안현재 버튼 스타일은 잘 구성되어 있지만, 하드코딩된 색상 값(#333333, #555555 등)을 사용하고 있습니다. 디자인 시스템 토큰이나 테마 변수를 사용하면 일관성과 유지보수성이 향상될 수 있습니다.
- background-color: #333333; + background-color: ${({ theme }) => theme.colors.primary};또한 switch 문 대신 객체 매핑을 사용하면 코드를 더 간결하게 만들 수 있습니다.
54-55: 색상 불투명도 표기 개선
rgba(51, 51, 51, 0.75)와rgba(51, 51, 51, 0.25)와 같은 불투명도 값은 일관된 디자인 시스템을 위해 변수로 추출하는 것이 좋습니다. 또한 색상 값 51은 16진수로 33이므로rgba(#333, 0.75)로 표현할 수도 있지만 Emotion에서는 지원되지 않아 현재 방식이 맞습니다.components/src/lib/components/Input/Input.styles.ts (1)
11-20:Details
❓ Verification inconclusive
색상 일관성 및 접근성 개선
에러 상태의 색상(#f44336)과 기본 상태의 색상(#ccc, #666)은 디자인 시스템 변수로 관리하는 것이 좋습니다. 또한 에러 메시지의 색상 대비가 접근성 표준(WCAG)을 충족하는지 확인해야 합니다.
🏁 Script executed:
#!/bin/bash # 접근성 관련 색상 대비 검사 도구 찾기 rg -g "*.{js,ts,tsx}" -i "accessibility|a11y|color.?contrast" --no-headingLength of output: 76
추가 확인: 디자인 시스템의 색상 토큰 정의 여부 및 코드 전반의 색상 사용 현황을 조사하기 위해 아래 스크립트를 실행해주세요.
🏁 Script executed:
#!/bin/bash set -e # 1) 색상 또는 테마 파일 검색 fd '\.(colors|theme)\.ts$' || true # 2) 에러 컬러(#f44336) 직접 사용 여부 검색 rg 'f4436' -g '*.{ts,tsx}' || true # 3) 컬러 토큰 정의(ex: export const errorColor) 검색 rg -IE 'export\s+const\s+\w*Color' -g '*.{ts,tsx}' || trueLength of output: 528
색상 토큰 사용 여부 확인 필요
현재 코드베이스에서
.colors.ts또는.theme.ts파일을 찾지 못했고,#f44336,#ccc,#666등의 하드코딩된 색상 사용을 검색했으나 별도의 컬러 토큰 정의를 확인할 수 없었습니다.아래 항목을 검토해 주세요:
- 디자인 시스템에서 제공하는 색상 토큰(예:
errorColor,neutral300등)이 존재하는지 확인- 존재한다면
border,border-color,box-shadow에 사용된 하드코딩 색상을 해당 토큰으로 교체- 에러 상태 색상 대비가 WCAG AA(최소 4.5:1) 기준을 만족하는지 검증
components/src/lib/components/Button/Button.tsx (1)
5-11: 타입 정의 개선ButtonProps 인터페이스가 잘 정의되어 있습니다. 다만, 각 prop에 JSDoc 주석을 추가하면 사용자가 컴포넌트를 더 쉽게 이해할 수 있습니다.
export interface ButtonProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { + /** 버튼 내부에 표시될 콘텐츠 */ children: React.ReactNode; + /** 버튼의 스타일 변형. 기본값: 'primary' */ variant?: ButtonVariant; + /** 버튼의 크기. 기본값: 'medium' */ size?: ButtonSize; + /** 버튼이 부모 컨테이너의 전체 너비를 차지할지 여부. 기본값: false */ fullWidth?: boolean; + /** 추가적인 CSS 클래스 */ className?: string; }components/src/lib/components/Modal/hooks/useModalContext.ts (2)
5-9: 타입 익스포트 개선
ModalContextType이 내부에서만 사용되는 타입이 아니라면, 외부에서도 사용할 수 있도록export키워드를 추가하는 것이 좋습니다. 이렇게 하면 소비자 코드에서 타입을 참조할 수 있어 유용합니다.-type ModalContextType = { +export type ModalContextType = { open: boolean; setOpen: React.Dispatch<React.SetStateAction<boolean>>; size?: ModalSize; };
13-20: 컨텍스트 사용 개선에러 메시지가 한국어로 되어 있어 국제화(i18n)가 필요한 경우 문제가 될 수 있습니다. 또한 다양한 시나리오에서 더 유용한 정보를 제공하는 상세한 에러 메시지를 고려해보세요.
if (!context) { - throw new Error('Modal 컴포넌트 내부에서만 사용할 수 있습니다.'); + throw new Error('useModalContext는 Modal 컴포넌트 또는 그 자식 컴포넌트 내부에서만 사용할 수 있습니다.'); }또한, 이 훅을 사용하는 컴포넌트에 대한 예제나 문서화를 추가하면 더 좋을 것 같습니다.
components/README.md (1)
1-290: 훌륭한 문서화 작업입니다만, 몇 가지 개선 사항을 제안합니다.README 문서의 구조와 내용이 매우 상세하게 작성되어 있어 좋습니다. 모달 컴포넌트의 특징, 설치 방법, API 및 사용 예제가 체계적으로 정리되어 있습니다. 다만, 아래 두 가지 개선 사항을 고려해보세요:
- 코드 예제에서 TypeScript 타입 정보가 포함된 버전도 제공하면 좋을 것 같습니다. 현재는 JSX만 있어 타입 사용법을 이해하기 어렵습니다.
- 접근성 섹션(281-289줄)에서 ARIA 속성과 역할(role)에 대한 정보도 추가하면 더 완벽한 접근성 가이드가 될 것입니다.
components/src/lib/components/Modal/Modal.stories.tsx (1)
79-295: 스토리 구현에서 반복되는 코드를 줄이고 사용자 상호작용을 개선할 수 있습니다.모달의 다양한 사용 사례를 잘 보여주는 스토리가 구현되어 있습니다. 그러나 다음과 같은 개선 사항을 고려해보세요:
Checkbox컴포넌트(79-84줄)가 체크 상태를 관리하지 않아 실제로 동작하지 않습니다. React 상태를 이용해 체크박스의 상태를 관리하는 것이 좋겠습니다.
SmallModal,MediumModal,LargeModal스토리에서ModalContent컴포넌트 코드가 반복됩니다. 이를 공통 컴포넌트로 분리하고 재사용하면 코드 중복을 줄일 수 있습니다.모든 모달 사이즈를 표시하는 단일 스토리를 만들거나, 크기를 매개변수로 받는 공통
SizedModal컴포넌트를 만드는 것이 좋겠습니다.components/src/lib/components/Modal/Modal.styles.ts (1)
87-90: 접근성 개선: 포커스 관리 최적화가 필요합니다.CloseButtonContainer의
outline: none스타일은 키보드 사용자의 접근성을 저해할 수 있습니다. 현재 box-shadow로 대체하고 있으나, 브라우저 기본 outline을 제거하지 않는 것이 접근성 측면에서 더 안전합니다.&:focus { - outline: none; box-shadow: 0 0 0 2px rgba(99, 102, 241, 0.3); }components/src/lib/components/Modal/Modal.tsx (4)
68-68: 중복된 기본값 설정 제거Modal 컴포넌트에서 이미 size 속성에 대한 기본값 'medium'을 설정했습니다. 그러나 Content 컴포넌트에서도 동일한 기본값을 중복해서 설정하고 있습니다. 이는 불필요한 중복으로 코드의 유지보수성을 해칠 수 있습니다.
-<ModalContentContainer - ref={contentRef} - className={className} - size={size || 'medium'} - role="dialog" - aria-modal="true" -> +<ModalContentContainer + ref={contentRef} + className={className} + size={size} + role="dialog" + aria-modal="true" +>Modal 컴포넌트에서 이미 기본값을 설정했기 때문에 Content 컴포넌트에서는 중복 설정이 필요하지 않습니다. Context 값을 그대로 사용하면 됩니다.
Also applies to: 20-20
69-71: 접근성 속성 개선 권장현재 모달은 기본적인 ARIA 속성을 가지고 있지만, 스크린 리더 사용자에게 더 나은 경험을 제공하기 위해서는 추가적인 ARIA 속성이 필요합니다.
<ModalContentContainer ref={contentRef} className={className} size={size || 'medium'} role="dialog" aria-modal="true" + aria-labelledby="modal-title" + aria-describedby="modal-description" >Title과 Description 컴포넌트에 각각 id를 부여하고, 이를 aria-labelledby와 aria-describedby에 연결하면 스크린 리더 사용자에게 모달의 제목과 설명을 명확하게 전달할 수 있습니다. 이는 웹 접근성 표준 준수에 중요합니다.
138-139: 하드코딩된 텍스트 상수화 및 국제화 고려버튼 텍스트가 한국어로 하드코딩되어 있습니다. 이는 다국어 지원이 필요할 경우 문제가 될 수 있습니다. 텍스트를 상수로 분리하고, 향후 국제화(i18n)를 지원할 수 있는 구조로 변경하는 것이 좋습니다.
+const ACTION_TEXT = { + confirm: '확인', + cancel: '취소', + default: '확인' +}; + +const ACTION_VARIANT = { + confirm: 'primary', + cancel: 'secondary', + default: 'primary' +}; function ActionButton({ ... }) { ... - const defaultText = action === 'confirm' ? '확인' : action === 'cancel' ? '취소' : '확인'; - const buttonVariant = action === 'confirm' ? 'primary' : action === 'cancel' ? 'secondary' : 'primary'; + const defaultText = ACTION_TEXT[action] || ACTION_TEXT.default; + const buttonVariant = ACTION_VARIANT[action] || ACTION_VARIANT.default; ... }이렇게 변경하면 코드의 가독성과 유지보수성이 향상되며, 추후 국제화를 적용하기 쉬워집니다.
30-54: Trigger 컴포넌트 로직 개선Trigger 컴포넌트의 asChild 패턴은 좋은 접근 방식이지만, 타입 캐스팅과 요소 복제(cloneElement) 부분을 개선할 수 있습니다. 현재 방식은 TypeScript의 타입 안전성을 제대로 활용하지 못하고 있습니다.
function Trigger({ children, className, asChild }: BaseProps & { asChild?: boolean; children?: React.ReactNode }) { const { setOpen } = useModalContext(); if (asChild && React.isValidElement(children)) { - const element = children as React.ReactElement<{ - onClick?: (e: React.MouseEvent) => void; - className?: string; - }>; + return React.cloneElement(children, { + ...children.props, + onClick: (e: React.MouseEvent) => { + children.props.onClick?.(e); + setOpen(true); + }, + className: `${children.props.className || ''} ${className || ''}`.trim(), + }); - return React.cloneElement(element, { - ...element.props, - onClick: (e: React.MouseEvent) => { - element.props.onClick?.(e); - setOpen(true); - }, - className: `${element.props.className || ''} ${className || ''}`.trim(), - }); } return ( <StyledButton className={className} onClick={() => setOpen(true)}> {children} </StyledButton> ); }
as키워드를 사용한 타입 캐스팅 대신 React.cloneElement에 바로 children을 전달함으로써 코드가 더 간결해지고 타입 안전성이 향상됩니다. React.isValidElement가 이미 유효한 React 요소인지 확인하기 때문에 추가 캐스팅은 불필요합니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
components/README.md(1 hunks)components/package.json(1 hunks)components/src/App.css(0 hunks)components/src/index.css(0 hunks)components/src/lib/Modal.stories.tsx(0 hunks)components/src/lib/Modal.styles.ts(0 hunks)components/src/lib/Modal.tsx(0 hunks)components/src/lib/components/Button/Button.styles.ts(1 hunks)components/src/lib/components/Button/Button.tsx(1 hunks)components/src/lib/components/Button/Button.types.ts(1 hunks)components/src/lib/components/Input/Input.styles.ts(1 hunks)components/src/lib/components/Input/Input.tsx(1 hunks)components/src/lib/components/Input/Input.types.ts(1 hunks)components/src/lib/components/Modal/CloseIcon.tsx(1 hunks)components/src/lib/components/Modal/Modal.stories.tsx(1 hunks)components/src/lib/components/Modal/Modal.styles.ts(1 hunks)components/src/lib/components/Modal/Modal.tsx(1 hunks)components/src/lib/components/Modal/Modal.types.ts(1 hunks)components/src/lib/components/Modal/hooks/useFocusTrap.ts(1 hunks)components/src/lib/components/Modal/hooks/useModalContext.ts(1 hunks)components/src/lib/index.ts(1 hunks)components/src/main.tsx(0 hunks)hooks/README.md(1 hunks)hooks/package.json(1 hunks)hooks/src/App.tsx(1 hunks)hooks/src/lib/constants/cardConfig.ts(0 hunks)hooks/src/lib/types/cardTypes.ts(1 hunks)hooks/src/lib/useCardCVC/useCardCVC.ts(1 hunks)hooks/src/lib/useCardNumber/cardRules.ts(1 hunks)hooks/src/lib/useCardNumber/identifyCardBrand.ts(1 hunks)hooks/src/lib/useCardNumber/useCardNumber.test.ts(1 hunks)hooks/src/lib/useCardNumber/useCardNumber.ts(1 hunks)hooks/src/lib/useCardPassword/useCardPassword.ts(1 hunks)
💤 Files with no reviewable changes (7)
- components/src/main.tsx
- hooks/src/lib/constants/cardConfig.ts
- components/src/App.css
- components/src/lib/Modal.stories.tsx
- components/src/index.css
- components/src/lib/Modal.tsx
- components/src/lib/Modal.styles.ts
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.ts`: You MUST follow these guidelines: - Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, re...
**/*.ts: You MUST follow these guidelines:
- Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, reference official documentation that can help resolve these issues
- Analyze the commit history to infer the cognitive process the developer went through while solving the problem. Summarize their thought progression: how they understood the problem, iterated on solutions, and refined the code. Focus on their reasoning patterns, learning moments, or shifts in approach. If applicable, relate their process to established best practices in the React ecosystem.
- Do not provide complete solutions unless explicitly requested.
- Guide the programmer in problem-solving instead of providing direct answers.
- When asked about programming concepts (e.g., "What is a hook?"), give direct and clear explanations.
- Break problems into smaller, manageable steps to help think through them logically.
- Ask leading questions and provide hints instead of just telling the answer.
- Encourage independent debugging before offering suggestions.
- Refer to relevant documentation instead of providing ready-made solutions.
- Promote modular thinking—breaking problems into reusable components.
- Remind the programmer to reflect on lessons learned after solving an issue.
- If explicitly asked for code (e.g., "Give me the code"), then provide it.
hooks/src/lib/useCardPassword/useCardPassword.tshooks/src/lib/types/cardTypes.tscomponents/src/lib/index.tshooks/src/lib/useCardCVC/useCardCVC.tscomponents/src/lib/components/Input/Input.types.tshooks/src/lib/useCardNumber/identifyCardBrand.tscomponents/src/lib/components/Button/Button.types.tscomponents/src/lib/components/Input/Input.styles.tscomponents/src/lib/components/Modal/Modal.types.tscomponents/src/lib/components/Button/Button.styles.tscomponents/src/lib/components/Modal/hooks/useModalContext.tshooks/src/lib/useCardNumber/useCardNumber.test.tscomponents/src/lib/components/Modal/hooks/useFocusTrap.tshooks/src/lib/useCardNumber/cardRules.tshooks/src/lib/useCardNumber/useCardNumber.tscomponents/src/lib/components/Modal/Modal.styles.ts
`**/*.tsx`: You MUST follow these guidelines: - Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, r...
**/*.tsx: You MUST follow these guidelines:
- Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, reference official documentation that can help resolve these issues
- Analyze the commit history to infer the cognitive process the developer went through while solving the problem. Summarize their thought progression: how they understood the problem, iterated on solutions, and refined the code. Focus on their reasoning patterns, learning moments, or shifts in approach. If applicable, relate their process to established best practices in the React ecosystem.
- Do not provide complete solutions unless explicitly requested.
- Guide the programmer in problem-solving instead of providing direct answers.
- When asked about programming concepts (e.g., "What is a hook?"), give direct and clear explanations.
- Break problems into smaller, manageable steps to help think through them logically.
- Ask leading questions and provide hints instead of just telling the answer.
- Encourage independent debugging before offering suggestions.
- Refer to relevant documentation instead of providing ready-made solutions.
- Promote modular thinking—breaking problems into reusable components.
- Remind the programmer to reflect on lessons learned after solving an issue.
- If explicitly asked for code (e.g., "Give me the code"), then provide it.
components/src/lib/components/Modal/CloseIcon.tsxcomponents/src/lib/components/Input/Input.tsxcomponents/src/lib/components/Button/Button.tsxhooks/src/App.tsxcomponents/src/lib/components/Modal/Modal.tsxcomponents/src/lib/components/Modal/Modal.stories.tsx
🧬 Code Graph Analysis (10)
hooks/src/lib/useCardNumber/identifyCardBrand.ts (1)
hooks/src/lib/useCardNumber/cardRules.ts (2)
CARD_RULES(10-53)CardBrandType(1-8)
components/src/lib/components/Input/Input.tsx (2)
components/src/lib/components/Input/Input.types.ts (1)
InputProps(1-5)components/src/lib/components/Input/Input.styles.ts (2)
StyledInputWrapper(32-32)StyledInput(32-32)
components/src/lib/components/Button/Button.tsx (2)
components/src/lib/components/Button/Button.types.ts (2)
ButtonVariant(1-1)ButtonSize(2-2)components/src/lib/components/Button/Button.styles.ts (1)
StyledButton(72-72)
components/src/lib/components/Modal/Modal.types.ts (1)
components/src/lib/components/Modal/hooks/useModalContext.ts (1)
ModalSize(3-3)
components/src/lib/components/Button/Button.styles.ts (2)
components/src/lib/components/Modal/Modal.styles.ts (1)
StyledButton(109-109)components/src/lib/components/Button/Button.types.ts (2)
ButtonVariant(1-1)ButtonSize(2-2)
hooks/src/lib/useCardNumber/useCardNumber.test.ts (2)
hooks/src/lib/useCardNumber/useCardNumber.ts (1)
useCardNumber(36-118)hooks/src/lib/constants/errorMessages.ts (1)
CARD_NUMBER_ERROR(3-6)
components/src/lib/components/Modal/Modal.tsx (6)
components/src/lib/index.ts (1)
Modal(1-1)components/src/lib/components/Modal/Modal.types.ts (3)
ModalProps(8-10)BaseProps(3-6)ActionButtonType(12-12)components/src/lib/components/Modal/hooks/useModalContext.ts (1)
ModalContext(11-11)components/src/lib/components/Modal/Modal.styles.ts (10)
ModalContainer(106-106)StyledButton(109-109)ModalOverlay(107-107)ModalContentContainer(108-108)HeaderContainer(110-110)TitleContainer(116-116)StyledTitle(111-111)StyledDescription(112-112)FooterContainer(113-113)CloseButtonContainer(114-114)components/src/lib/components/Modal/CloseIcon.tsx (1)
CloseIcon(1-11)components/src/lib/components/Button/Button.tsx (1)
Button(13-21)
hooks/src/lib/useCardNumber/useCardNumber.ts (4)
hooks/src/lib/useCardNumber/cardRules.ts (1)
CARD_RULES(10-53)hooks/src/lib/index.ts (2)
useCardNumber(4-4)isOnlyDigits(8-8)hooks/src/lib/utils/validateNumber.ts (1)
isOnlyDigits(1-1)hooks/src/lib/constants/errorMessages.ts (1)
CARD_NUMBER_ERROR(3-6)
components/src/lib/components/Modal/Modal.stories.tsx (4)
components/src/lib/components/Modal/Modal.tsx (1)
Modal(173-173)components/src/lib/index.ts (1)
Modal(1-1)components/src/lib/components/Button/Button.tsx (1)
Button(13-21)components/src/lib/components/Input/Input.tsx (1)
Input(5-13)
components/src/lib/components/Modal/Modal.styles.ts (1)
components/src/lib/components/Modal/hooks/useModalContext.ts (1)
ModalSize(3-3)
🔇 Additional comments (19)
hooks/src/lib/useCardPassword/useCardPassword.ts (1)
5-7: 상수 모듈화 접근 방식상수를 각 파일 내부로 이동한 리팩토링 접근법은 모듈화 관점에서 좋은 변화입니다. 각 훅이 자체적으로 필요한 상수를 정의함으로써 의존성이 줄어들고 응집도가 높아집니다.
hooks/src/lib/useCardCVC/useCardCVC.ts (1)
5-7: 상수 모듈화 접근 방식상수를 각 파일 내부로 이동한 리팩토링 접근법은 모듈화 관점에서 좋은 변화입니다. 각 훅이 자체적으로 필요한 상수를 정의함으로써 의존성이 줄어들고 응집도가 높아집니다.
hooks/src/lib/useCardNumber/useCardNumber.test.ts (4)
10-16: 초기값 테스트가 적절히 구현되었습니다.
useCardNumber훅의 초기값 설정 테스트가 잘 작성되었습니다. 문자열 형태의 카드 번호가 올바르게 세그먼트 배열로 변환되는지 확인하고 있습니다.
18-28: 입력 핸들링 테스트가 명확합니다.숫자 입력 시 상태 업데이트와 에러 메시지 처리에 대한 테스트가 잘 구현되었습니다.
idx매개변수를 통한 세그먼트 인덱스 기반 접근 방식이 적절합니다.
69-81: 카드 브랜드 감지 테스트가 잘 작성되었습니다.다양한 카드 브랜드(VISA, MASTERCARD, AMEX, UNKNOWN)에 대한 감지 테스트가 포괄적으로 구현되어 있습니다. 각 브랜드의 첫 번째 숫자 패턴에 따른 감지 로직이 잘 테스트되고 있습니다.
83-93: 필드 구성 테스트가 명확합니다.각 카드 브랜드에 따른 필드 수와 길이 설정이 올바르게 테스트되고 있습니다. VISA와 AMEX 카드의 다른 필드 구성을 검증하는 것은 중요한 테스트 케이스입니다.
hooks/README.md (3)
1-18: README 문서 내용이 포괄적으로 개선되었습니다.훅의 주요 기능을 명확하게 나열하고 있어 사용자가 기능을 이해하기 쉽습니다. 자동 카드 브랜드 감지, 유효성 검사, 다중 필드 지원 등 핵심 기능이 잘 설명되어 있습니다.
19-32: 설치 지침이 명확합니다.npm과 yarn을 통한 설치 방법이 명확하게 제시되어 있어 사용자가 쉽게 패키지를 설치할 수 있습니다.
86-113: API 참조가 상세하게 작성되었습니다.훅의 매개변수와 반환 값에 대한 설명이 표 형식으로 잘 정리되어 있습니다. 각 항목의 설명이 명확하여 사용자가 쉽게 이해할 수 있습니다.
hooks/src/App.tsx (2)
1-17: useCardNumber 훅 활용이 잘 이루어졌습니다.
useCardNumber훅을 적절히 임포트하고 초기값과 함께 호출하고 있습니다. 반환된 모든 값들이 명확하게 구조 분해 할당되어 있어 코드 가독성이 좋습니다.
20-29:Details
✅ Verification successful
동적 입력 필드 렌더링이 잘 구현되었습니다.
requiredFields와fieldLengthArr를 활용하여 카드 브랜드에 따라 동적으로 입력 필드를 렌더링하는 방식이 효과적입니다. 각 입력 필드는 적절한 핸들러와 속성을 가지고 있습니다.입력 필드가 비어있을 때
formattedCardNumber[idx]가 undefined일 경우를 대비해|| ''를 추가한 것은 좋은 방어 코딩입니다. 다만, 훅 내부에서 이 부분이 처리되는지 확인해보는 것이 좋을 것 같습니다.
🏁 Script executed:
#!/bin/bash # 훅이 undefined 값을 어떻게 처리하는지 확인 rg -A 5 "setFormattedCardNumber" hooks/src/lib/useCardNumber/useCardNumber.tsLength of output: 877
🏁 Script executed:
#!/bin/bash # splitInitialCardNumber 구현 내용 확인 rg -A10 "splitInitialCardNumber" hooks/src/lib/useCardNumber/useCardNumber.tsLength of output: 1086
|| ''방어 코딩 불필요하지만 안전함 – 확인 완료
splitInitialCardNumber가 항상CARD_NUMBER.fieldCount길이만큼 빈 문자열로 배열을 채워 반환하므로formattedCardNumber[idx]가undefined가 될 일은 없습니다. 동적 입력 필드 렌더링 구현도 적절하며, 추가 검증이나 수정 사항은 없습니다.components/package.json (1)
2-2: 패키지 이름 변경 승인패키지 이름이 "lumes_modal"에서 "@suhwa/react-modal"로 변경되었습니다. 이는 PR 목표에 맞게 스코프가 있는 일관된 네이밍 전략을 적용한 것으로 보입니다.
components/src/lib/index.ts (1)
1-1:Details
✅ Verification successful
모달 컴포넌트 내보내기 변경 사항
모달 컴포넌트의 내보내기가 기본 내보내기(default export)에서 명명된 내보내기(named export)로 변경되었고, 파일 경로도 './Modal'에서 './components/Modal/Modal'로 변경되었습니다. 이는 모달 컴포넌트의 구조적 리팩토링을 반영하는 좋은 변화입니다.
이 변경으로 인해 라이브러리 소비자들은 기존 임포트 방식을 업데이트해야 할 수 있습니다. 이런 변경에 대해서는 CHANGELOG나 마이그레이션 가이드를 제공하는 것이 좋습니다.
🏁 Script executed:
#!/bin/bash # 기존 임포트 방식을 사용하는 코드가 있는지 확인 rg "import Modal from" --type ts --type tsxLength of output: 73
모달의 기본 임포트 사용 여부를 정확히 확인하기 위해
.ts와.tsx파일을 명시적으로 검색하도록 수정합니다.
🏁 Script executed:
#!/bin/bash # .ts 및 .tsx 파일에서 기본(default) 임포트 사용 확인 rg "import Modal from" -g '*.ts' -g '*.tsx'Length of output: 45
모달 컴포넌트 내보내기 변경 확인 및 영향 범위
export { Modal } from './components/Modal/Modal';로의 내보내기 변경으로, 기존의 기본(default) 임포트는 더 이상 지원되지 않습니다..ts및.tsx파일 전체를 검색한 결과(import Modal from), 기본 임포트 사용 구문은 발견되지 않아, 기존 코드에 즉각적인 오류는 발생하지 않을 것으로 보입니다.- 그래도 모달 컴포넌트의 내보내기 방식이 변경된 만큼, 사용자 가이드를 위해 CHANGELOG 또는 마이그레이션 가이드를 함께 제공하는 것을 권장합니다.
components/src/lib/components/Modal/CloseIcon.tsx (1)
1-11: 개발자의 모달 시스템 리팩토링 과정 분석코드 변경 사항을 살펴보면, 개발자가 단순한 모달 컴포넌트에서 복합 모달 시스템으로 발전시키는 과정을 거쳤음을 알 수 있습니다. 처음에는 기본적인 모달 구현에서 시작하여, 점진적으로 컴포지션 패턴을 적용해 더 모듈화되고 재사용 가능한 구조로 발전시켰습니다.
특히 CloseIcon 같은 UI 요소를 별도의 컴포넌트로 분리함으로써 관심사 분리 원칙을 적용하고, 각 컴포넌트가 단일 책임을 갖도록 설계했습니다. 이는 React의 컴포넌트 기반 아키텍처의 장점을 최대한 활용한 좋은 접근 방식입니다.
이러한 변경은 단순히 코드 리팩토링을 넘어, React 생태계의 현대적인 컴포넌트 설계 방식(Compound Components 패턴)을 학습하고 적용한 결과로 보입니다. 이는 React 커뮤니티에서 널리 사용되는 디자인 패턴으로, 코드의 유지보수성과 재사용성을 크게 향상시킵니다.
components/src/lib/components/Modal/Modal.types.ts (1)
12-12: 액션 버튼 타입이 잘 정의되어 있습니다.버튼 액션 타입을 명확하게 구분한 것이 좋습니다. 다만, 각 타입의 의미가 명확하도록 주석이나 문서화를 추가하면 더 좋을 것 같습니다. 타입의 사용 목적과 각 값의 의미를 설명해주세요.
/** * 모달 내 액션 버튼의 타입 * - confirm: 긍정적인 액션(예: 확인, 제출) * - cancel: 부정적인 액션(예: 취소, 닫기) * - default: 기본 액션(중립적) */ export type ActionButtonType = 'confirm' | 'cancel' | 'default';components/src/lib/components/Button/Button.tsx (1)
13-21: Button 컴포넌트 구현이 잘 되었습니다React.forwardRef를 사용하여 ref를 전달하는 방식과 기본 props 설정 방법이 적절합니다. children과 나머지 props를 전달하는 패턴도 훌륭합니다.
다만, 훅 패턴을 활용해 버튼의 동작 로직을 분리하는 것을 고려해볼 수 있습니다. 예를 들어, 로딩 상태나 비활성화 상태를 관리하는 훅을 별도로 구현할 수 있습니다.
components/src/lib/components/Modal/hooks/useModalContext.ts (1)
11-11:Details
❓ Verification inconclusive
초기값 개선 제안
ModalContext의 초기값으로undefined를 사용하는 것은 일반적인 패턴이지만, 타입 안전성을 높이기 위해 빈 객체 또는 기본값을 제공하는 방식도 고려해볼 수 있습니다. 다만 이 경우!context검사 로직을 조정해야 합니다.
🌐 Web query:
React Context API에서 기본값을 undefined 대신 mock 객체로 제공하는 것의 장단점은 무엇인가요?💡 Result:
React Context API에서 기본값을 undefined 대신 mock 객체로 제공하는 것의 장단점
장점
Provider 누락 시 안전성 확보
컴포넌트 트리에 Provider가 없을 때 undefined가 아닌 mock 객체(또는 의미 있는 기본값)를 사용하면, 하위 컴포넌트에서 context 값을 안전하게 사용할 수 있습니다. 이로 인해 null/undefined 체크 코드가 줄고, 런타임 오류 가능성이 줄어듭니다[4][7].테스트 용이성
Provider를 따로 설정하지 않고 독립적으로 컴포넌트를 테스트할 때, mock 객체를 기본값으로 지정하면 실제 Provider를 감싸지 않아도 원하는 동작을 모의하여 테스트가 간편해집니다[4][7].IDE 자동완성 및 타입 안전성
mock 객체에 예상되는 모든 속성/함수를 포함시켜 타입스크립트 등의 환경에서 컴파일 타임에 타입 검증이 이뤄집니다. undefined를 기본값으로 주면 타입이 불명확해질 수 있습니다[7].개발 생산성 향상
context 구조를 명확하게 정의하고, 모든 필수 값 및 함수가 mock 객체에 들어 있으므로, 코드 작성 시 실수(예: context가 undefined임을 간과하는 등)를 줄일 수 있습니다[7].단점
실제 환경과 다른 동작 위험
Provider가 누락되어도 mock 객체가 동작하므로, 개발자가 Provider를 감싸지 않아도 문제가 없는 것처럼 오해할 수 있습니다. Provider로 context를 주입해야 하는데 실수로 생략될 수 있으며, 이 경우에도 명확한 오류가 발생하지 않습니다[6][10].조용한 버그 가능성
실제로는 Provider가 필요하지만, mock 객체로 인해 예상과 다른 값이 동작하다가, 배포 후 버그가 발견될 수 있습니다. 이런 실수를 막으려면 일부러 undefined를 기본값으로 두고, useContext(CustomContext) 사용 시 undefined일 때 에러를 발생시키는 패턴을 선호하기도 합니다[6][10].mock 함수 오남용 위험
context에 들어가는 mock 함수가 호출되어도 실제 아무런 동작을 하지 않을 수 있어, 이런 부분이 테스트 단계에서 누락될 수 있습니다[7].정리
mock 객체를 기본값으로 사용 undefined를 기본값으로 사용 Provider 누락 시 안전하게 동작, 오류 없음 런타임 오류, 명확한 문제 노출 테스트 용이성 높음 (별도의 Provider 필요 없음) Provider를 Mock으로 감싸야 함 코드 안전성/가독성 높음 (null 검사 감소, 타입 명확) null 체크 필요, 타입 불명확 가능 실수 발견 용이성 낮음 (실수 숨김) 높음 (실수 즉시 노출) 배포 시 안전성 mock 값으로 알 수 없는 문제 가능 즉시 오류 발생으로 문제 조기 발견 blockqoute
컨텍스트 기본값은 Provider 없이 컴포넌트를 테스트할 때 유용하며, 기본값으로 undefined를 넣으면 기본값이 사용되지 않는다[4].
결론
- mock 객체를 기본값으로 사용하면 테스트 및 개발 편의성이 높고, 안전하게 사용할 수 있으나, Provider 누락 시 조용한 버그가 생길 수 있습니다.
- undefined를 기본값으로 사용하면 Provider를 반드시 감싸도록 강제할 수 있고, 실수를 즉시 노출하는 데 효과적입니다.
- 프로젝트 규모, 컨텍스트의 중요성, 테스트 패턴에 따라 적절한 방식을 선택하는 것이 좋습니다. 중요한 컨텍스트라면 undefined를, 독립 테스트가 중요한 경우 mock 객체도 좋은 선택입니다.
Citations:
- 1: https://velog.io/@function_dh/React-Context-API-%EC%9D%98%EC%A1%B4%EC%84%B1-%EC%A3%BC%EC%9E%85%EC%97%90-%EA%B4%80%ED%95%98%EC%97%AC
- 2: https://lasbe.tistory.com/166
- 3: https://velog.io/@khy226/Context-API%EB%A5%BC-%EC%82%AC%EC%9A%A9%ED%95%9C-%EC%83%81%ED%83%9C%EA%B4%80%EB%A6%AC
- 4: https://un-gaemii.tistory.com/26
- 5: https://ko.react.dev/reference/react/createContext
- 6: https://velog.io/@velopert/react-context-tutorial
- 7: https://junheedot.tistory.com/entry/%EC%98%88%EC%A0%9C%EB%A1%9C-%EB%B0%B0%EC%9A%B0%EB%8A%94-react-context
- 8: https://www.datoybi.com/context-api/
- 9: https://ko.react.dev/reference/react/useContext
- 10: https://yesolz.tistory.com/entry/React-Context-API
ModalContext 초기값 선택 시 장단점 검토
createContext<ModalContextType | undefined>(undefined)패턴은 Provider 누락 시 즉시 오류를 발생시켜 실수를 빠르게 발견할 수 있다는 장점이 있지만, null 검사 코드가 반복되고 타입 안전성이 다소 떨어질 수 있습니다. 반면 mock 객체(또는 의미 있는 기본값)를 제공하면 런타임 안전성과 테스트 편의성이 높아지나, Provider 누락 시 조용한 버그가 생길 위험이 있습니다.주요 비교:
mock 객체 기본값 사용
• Provider 누락해도 안전하게 동작 → null/undefined 검사 제거 가능
• 독립 테스트 시 별도 Provider 없이도 사용 용이
• IDE 자동완성·컴파일 타임 타입 검증 강화
— 그러나 Provider 미설정 실수가 눈에 띄지 않고, 배포 후 예기치 않은 동작 발생 가능undefined 기본값 사용
• Provider 누락 시 즉시 오류 발생 → 실수 조기 발견
• 불필요한 mock 값 오용 방지
— 그러나 하위 컴포넌트에서 매번 null/undefined 검사 필요프로젝트 규모, ModalContext의 중요도, 테스트 패턴 등을 고려해 적절한 초기값 방식을 선택하고, mock 객체를 사용한다면
useModalContext내부의 undefined 검사 로직을 기본값에 맞게 조정해주세요.components/src/lib/components/Modal/Modal.stories.tsx (1)
79-91: 🛠️ Refactor suggestion체크박스 컴포넌트에 상태 관리 추가가 필요합니다.
현재
Checkbox컴포넌트는 상태 관리가 없어 체크/언체크 기능이 동작하지 않습니다. 실제 사용성을 보여주기 위해 상태 관리를 추가하는 것이 좋겠습니다.-const Checkbox = ({ label }: { label: string }) => ( +const Checkbox = ({ label }: { label: string }) => { + const [checked, setChecked] = useState(false); + + return ( <div style={{ display: 'flex', alignItems: 'center', marginBottom: '12px' }}> - <input type="checkbox" id={label} style={{ marginRight: '8px' }} /> + <input + type="checkbox" + id={label} + style={{ marginRight: '8px' }} + checked={checked} + onChange={() => setChecked(!checked)} + /> <label htmlFor={label}>{label}</label> </div> -); + ); +};Likely an incorrect or invalid review comment.
components/src/lib/components/Modal/Modal.tsx (1)
1-173: 모달 컴포넌트 설계에 대한 전반적인 피드백전체적으로 컴파운드 컴포넌트 패턴을 사용한 모달 구현이 잘 되어 있습니다. React 컨텍스트를 활용하여 상태를 공유하고, 각 서브컴포넌트가 명확한 역할을 가지도록 구성한 점이 좋습니다. 특히 Trigger 컴포넌트의 asChild 패턴을 통한 유연성과 ActionButton을 통한 다양한 액션 지원은 사용성을 크게 향상시킵니다.
다만 개선할 수 있는 몇 가지 영역이 있습니다:
- 접근성 측면에서 aria 속성과 키보드 상호작용을 더 강화하세요.
- 국제화(i18n)를 고려하여 하드코딩된 텍스트를 상수 또는 설정으로 분리하세요.
- 컴포넌트 간의 책임 분리를 명확히 하여 단일 책임 원칙(SRP)을 더 잘 준수하도록 하세요.
- 일부 중복된 로직과 불필요한 타입 캐스팅을 제거하여 코드 가독성을 개선하세요.
위 수정사항을 반영하면 더 견고하고 유지보수하기 쉬운 모달 컴포넌트가 될 것입니다.
| const identifyCardBrand = (cardNumber: string) => { | ||
| if (CARD_RULES[CardBrandType.VISA].pattern.test(cardNumber)) { | ||
| return CardBrandType.VISA; | ||
| } else if (CARD_RULES[CardBrandType.MASTERCARD].pattern.test(cardNumber)) { | ||
| return CardBrandType.MASTERCARD; | ||
| } else if (CARD_RULES[CardBrandType.AMEX].pattern.test(cardNumber)) { | ||
| return CardBrandType.AMEX; | ||
| } else if (CARD_RULES[CardBrandType.DINERS].pattern.test(cardNumber)) { | ||
| return CardBrandType.DINERS; | ||
| } else if (CARD_RULES[CardBrandType.UNIONPAY].pattern.test(cardNumber)) { | ||
| return CardBrandType.UNIONPAY; | ||
| } | ||
| return CardBrandType.UNKNOWN; | ||
| }; | ||
|
|
||
| export default identifyCardBrand; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
타입 안전성 개선 제안
현재 함수는 반환 타입이 명시적으로 정의되어 있지 않습니다. TypeScript의 장점을 최대한 활용하기 위해 반환 타입을 명시하고, 입력값에 대한 유효성 검사를 추가하는 것이 좋습니다.
- const identifyCardBrand = (cardNumber: string) => {
+ const identifyCardBrand = (cardNumber: string): keyof typeof CardBrandType => {
+ if (!cardNumber) {
+ return CardBrandType.UNKNOWN;
+ }
+
// 기존 식별 로직
return CardBrandType.UNKNOWN;
};이렇게 하면 함수의 인터페이스가 더 명확해지고, 타입 안전성이 개선됩니다. 또한 빈 문자열이나 null/undefined 값이 전달되는 예외 상황도 처리할 수 있습니다.
🤖 Prompt for AI Agents (early access)
In hooks/src/lib/useCardNumber/identifyCardBrand.ts around lines 3 to 18, the
identifyCardBrand function lacks an explicit return type and does not validate
input values. To fix this, explicitly define the function's return type as
CardBrandType and add input validation to handle cases where cardNumber is an
empty string, null, or undefined. Return CardBrandType.UNKNOWN for invalid
inputs to improve type safety and function clarity.
| export const CARD_RULES = { | ||
| [CardBrandType.VISA]: { | ||
| lengths: [16], | ||
| pattern: /^4/, | ||
| formatPattern: /(\d{4})(\d{4})(\d{4})(\d{4})/, | ||
| fields: 4, | ||
| fieldLengths: [4, 4, 4, 4], | ||
| }, | ||
| [CardBrandType.MASTERCARD]: { | ||
| lengths: [16], | ||
| pattern: /^(5[1-5]|2[2-7])/, | ||
| formatPattern: /(\d{4})(\d{4})(\d{4})(\d{4})/, | ||
| fields: 4, | ||
| fieldLengths: [4, 4, 4, 4], | ||
| }, | ||
| [CardBrandType.AMEX]: { | ||
| lengths: [15], | ||
| pattern: /^3[47]/, | ||
| formatPattern: /(\d{4})(\d{6})(\d{5})/, | ||
| fields: 3, | ||
| fieldLengths: [4, 6, 5], | ||
| }, | ||
| [CardBrandType.DINERS]: { | ||
| lengths: [14], | ||
| pattern: /^36/, | ||
| formatPattern: /(\d{4})(\d{6})(\d{4})/, | ||
| fields: 3, | ||
| fieldLengths: [4, 6, 4], | ||
| }, | ||
| [CardBrandType.UNIONPAY]: { | ||
| lengths: [16], | ||
| pattern: /^(62212[6-9]|6221[3-9]|622[2-8]|6229[0-1]|62292[0-5])/, | ||
| formatPattern: /(\d{4})(\d{4})(\d{4})(\d{4})/, | ||
| fields: 4, | ||
| fieldLengths: [4, 4, 4, 4], | ||
| }, | ||
| [CardBrandType.UNKNOWN]: { | ||
| lengths: [16], | ||
| pattern: /^\d+/, | ||
| formatPattern: /(\d{4})(\d{4})(\d{4})(\d{4})/, | ||
| fields: 4, | ||
| fieldLengths: [4, 4, 4, 4], | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
카드 규칙에 대한 인터페이스 정의 제안
CARD_RULES 객체는 구조화된 데이터를 포함하고 있지만, 명시적인 타입 정의가 없습니다. TypeScript 인터페이스를 추가하여 규칙의 구조를 문서화하고 타입 안전성을 개선할 수 있습니다.
먼저 규칙에 대한 인터페이스를 정의합니다:
interface CardRule {
lengths: number[];
pattern: RegExp;
formatPattern: RegExp;
fields: number;
fieldLengths: number[];
}
export type CardRules = Record<keyof typeof CardBrandType, CardRule>;그런 다음 CARD_RULES를 이 타입으로 정의합니다:
- export const CARD_RULES = {
+ export const CARD_RULES: CardRules = {
// 기존 규칙들...
};이렇게 하면 타입 안전성이 높아지고, 각 카드 규칙이 필요한 모든 속성을 갖추고 있는지 컴파일 시간에 확인할 수 있습니다.
🤖 Prompt for AI Agents (early access)
In hooks/src/lib/useCardNumber/cardRules.ts around lines 10 to 53, the
CARD_RULES object lacks an explicit TypeScript interface, reducing type safety
and clarity. Define a CardRule interface specifying lengths, pattern,
formatPattern, fields, and fieldLengths properties, then create a CardRules type
as a record keyed by CardBrandType with CardRule values. Finally, type the
CARD_RULES constant with this CardRules type to ensure all card rules conform to
the expected structure at compile time.
| const formatPattern = CARD_RULES[cardBrand].formatPattern; | ||
| if (!formatPattern.test(cardNumber)) { | ||
| return false; | ||
| } | ||
|
|
||
| return cardNumber.length === expectedTotalLength; |
There was a problem hiding this comment.
카드 번호 형식 패턴이 전체 문자열을 검증하지 못합니다
formatPattern.test(cardNumber)에서는 formatPattern(예: /(\d{4})(\d{4})(\d{4})(\d{4})/)이 문자열 전체가 아닌 “일치하는 부분 문자열”만 찾아도 true를 반환합니다.
예를 들어 1234처럼 짧은 입력도 부분 일치로 통과할 수 있어, 실사용 시 잘못된 카드 번호가 유효하다고 판단될 위험이 있습니다.
-const formatPattern = CARD_RULES[cardBrand].formatPattern;
-if (!formatPattern.test(cardNumber)) {
+const formatPattern = CARD_RULES[cardBrand].formatPattern;
+if (!new RegExp(`^${formatPattern.source}$`).test(cardNumber)) {
return false;
}또는 cardRules.ts 내 각 formatPattern에 ^ 와 $를 미리 포함해 두는 방법도 있습니다.
이 패턴 실수는 다른 곳에서도 반복될 가능성이 있으니, 정규식 사용 시 항상 “전체 일치” 여부를 확인하는 습관을 들이시는 것을 권장드립니다.
📝 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 formatPattern = CARD_RULES[cardBrand].formatPattern; | |
| if (!formatPattern.test(cardNumber)) { | |
| return false; | |
| } | |
| return cardNumber.length === expectedTotalLength; | |
| const formatPattern = CARD_RULES[cardBrand].formatPattern; | |
| if (!new RegExp(`^${formatPattern.source}$`).test(cardNumber)) { | |
| return false; | |
| } | |
| return cardNumber.length === expectedTotalLength; |
🤖 Prompt for AI Agents (early access)
In hooks/src/lib/useCardNumber/useCardNumber.ts around lines 85 to 90, the
formatPattern regular expression does not enforce full string matching, causing
partial matches to incorrectly pass validation. To fix this, update the test to
ensure the entire cardNumber matches the pattern by either adding start (^) and
end ($) anchors to the regex in CARD_RULES or by using a method that checks for
full string match. This change will prevent short or partial inputs from being
mistakenly validated as correct card numbers.
hooks/package.json
Outdated
| "name": "@suhwa/react-custom-hooks", | ||
| "version": "0.0.2", |
There was a problem hiding this comment.
패키지 이름 변경과 관련된 일관성 문제
패키지 이름이 "lume_hooks"에서 "@suhwa/react-custom-hooks"로 변경되었고 버전이 증가되었습니다. 하지만 21번 라인에서는 여전히 "lume_hooks"를 의존성으로 가지고 있습니다. 이는 잠재적으로 순환 의존성 문제를 일으킬 수 있습니다.
"dependencies": {
- "lume_hooks": "^0.0.1",
"react": "^19.1.0",
"react-dom": "^19.1.0"
},자신의 이전 패키지 이름을 의존성으로 갖는 것은 불필요하며, 빌드 및 배포 과정에서 문제를 일으킬 수 있습니다.
Also applies to: 21-21
🤖 Prompt for AI Agents (early access)
In hooks/package.json at lines 2-3 and line 21, the package name has been
updated to "@suhwa/react-custom-hooks" but the dependency at line 21 still
references the old name "lume_hooks". To fix this, update the dependency at line
21 to use the new package name "@suhwa/react-custom-hooks" to maintain
consistency and avoid potential circular dependency issues.
| import { useCallback, useEffect, useRef } from 'react'; | ||
|
|
||
| function useFocusTrap(open: boolean, setOpen: (open: boolean) => void) { | ||
| const contentRef = useRef<HTMLDivElement>(null); | ||
| const previousFocusRef = useRef<HTMLElement | null>(null); | ||
|
|
||
| const getFocusableElements = useCallback(() => { | ||
| if (!contentRef.current) return []; | ||
|
|
||
| const focusableSelectors = ['button', '[href]', 'input', 'select', 'textarea', '[tabindex]:not([tabindex="-1"])']; | ||
|
|
||
| const elements = contentRef.current.querySelectorAll<HTMLElement>(focusableSelectors.join(',')); | ||
|
|
||
| return Array.from(elements); | ||
| }, []); | ||
|
|
||
| const handleKeyDown = useCallback( | ||
| (e: KeyboardEvent) => { | ||
| if (e.key === 'Escape') { | ||
| setOpen(false); | ||
| return; | ||
| } | ||
|
|
||
| if (e.key === 'Tab') { | ||
| const focusableElements = getFocusableElements(); | ||
| if (focusableElements.length === 0) return; | ||
|
|
||
| const firstElement = focusableElements[0]; | ||
| const lastElement = focusableElements[focusableElements.length - 1]; | ||
|
|
||
| if (e.shiftKey) { | ||
| if (document.activeElement === firstElement) { | ||
| e.preventDefault(); | ||
| lastElement.focus(); | ||
| } | ||
| } else { | ||
| if (document.activeElement === lastElement) { | ||
| e.preventDefault(); | ||
| firstElement.focus(); | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| [getFocusableElements, setOpen], | ||
| ); | ||
|
|
||
| const setupInitialFocus = useCallback(() => { | ||
| const focusableElements = getFocusableElements(); | ||
| if (focusableElements.length > 0) { | ||
| focusableElements[0].focus(); | ||
| } else if (contentRef.current) { | ||
| contentRef.current.setAttribute('tabindex', '-1'); | ||
| contentRef.current.focus(); | ||
| } | ||
| }, [getFocusableElements]); | ||
|
|
||
| useEffect(() => { | ||
| if (open) { | ||
| // 모달이 열리기 전 현재 포커스된 요소 저장 | ||
| previousFocusRef.current = document.activeElement as HTMLElement; | ||
|
|
||
| // 키보드 이벤트 리스너 등록 | ||
| document.addEventListener('keydown', handleKeyDown); | ||
|
|
||
| // 첫 번째 요소에 포커스 - 약간의 지연을 두어 DOM이 완전히 렌더링된 후 실행 | ||
| setTimeout(setupInitialFocus, 0); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| }; | ||
| } else if (previousFocusRef.current) { | ||
| // 모달이 닫힐 때 이전 포커스 요소로 복귀 | ||
| previousFocusRef.current.focus(); | ||
| } | ||
| }, [open, handleKeyDown, setupInitialFocus]); | ||
|
|
||
| return contentRef; | ||
| } | ||
|
|
||
| export default useFocusTrap; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
접근성을 위한 훌륭한 구현이지만, 몇 가지 개선점이 있습니다.
useFocusTrap 훅은 모달의 접근성을 위한 중요한 기능을 구현했습니다. 키보드 탐색과 포커스 트래핑을 잘 처리하고 있습니다. 다만 다음과 같은 개선 사항을 고려해보세요:
setTimeout을 사용하는 라인 66에서 컴포넌트가 언마운트되기 전에 타임아웃이 완료되지 않을 경우를 대비한 클린업 함수가 없습니다.- 라인 52에서
contentRef.current.setAttribute('tabindex', '-1')는 모달 컨텐츠가 이미 tabindex를 가지고 있을 경우 문제가 될 수 있습니다. 설정 전에 기존 tabindex 확인이 필요합니다.
참고: React 공식 문서의 타이머 정리하기 섹션에서 setTimeout 정리 방법을 확인하세요.
useEffect(() => {
if (open) {
// 모달이 열리기 전 현재 포커스된 요소 저장
previousFocusRef.current = document.activeElement as HTMLElement;
// 키보드 이벤트 리스너 등록
document.addEventListener('keydown', handleKeyDown);
// 첫 번째 요소에 포커스 - 약간의 지연을 두어 DOM이 완전히 렌더링된 후 실행
- setTimeout(setupInitialFocus, 0);
+ const timeoutId = setTimeout(setupInitialFocus, 0);
return () => {
document.removeEventListener('keydown', handleKeyDown);
+ clearTimeout(timeoutId);
};
} else if (previousFocusRef.current) {
// 모달이 닫힐 때 이전 포커스 요소로 복귀
previousFocusRef.current.focus();
}
}, [open, handleKeyDown, setupInitialFocus]);📝 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.
| import { useCallback, useEffect, useRef } from 'react'; | |
| function useFocusTrap(open: boolean, setOpen: (open: boolean) => void) { | |
| const contentRef = useRef<HTMLDivElement>(null); | |
| const previousFocusRef = useRef<HTMLElement | null>(null); | |
| const getFocusableElements = useCallback(() => { | |
| if (!contentRef.current) return []; | |
| const focusableSelectors = ['button', '[href]', 'input', 'select', 'textarea', '[tabindex]:not([tabindex="-1"])']; | |
| const elements = contentRef.current.querySelectorAll<HTMLElement>(focusableSelectors.join(',')); | |
| return Array.from(elements); | |
| }, []); | |
| const handleKeyDown = useCallback( | |
| (e: KeyboardEvent) => { | |
| if (e.key === 'Escape') { | |
| setOpen(false); | |
| return; | |
| } | |
| if (e.key === 'Tab') { | |
| const focusableElements = getFocusableElements(); | |
| if (focusableElements.length === 0) return; | |
| const firstElement = focusableElements[0]; | |
| const lastElement = focusableElements[focusableElements.length - 1]; | |
| if (e.shiftKey) { | |
| if (document.activeElement === firstElement) { | |
| e.preventDefault(); | |
| lastElement.focus(); | |
| } | |
| } else { | |
| if (document.activeElement === lastElement) { | |
| e.preventDefault(); | |
| firstElement.focus(); | |
| } | |
| } | |
| } | |
| }, | |
| [getFocusableElements, setOpen], | |
| ); | |
| const setupInitialFocus = useCallback(() => { | |
| const focusableElements = getFocusableElements(); | |
| if (focusableElements.length > 0) { | |
| focusableElements[0].focus(); | |
| } else if (contentRef.current) { | |
| contentRef.current.setAttribute('tabindex', '-1'); | |
| contentRef.current.focus(); | |
| } | |
| }, [getFocusableElements]); | |
| useEffect(() => { | |
| if (open) { | |
| // 모달이 열리기 전 현재 포커스된 요소 저장 | |
| previousFocusRef.current = document.activeElement as HTMLElement; | |
| // 키보드 이벤트 리스너 등록 | |
| document.addEventListener('keydown', handleKeyDown); | |
| // 첫 번째 요소에 포커스 - 약간의 지연을 두어 DOM이 완전히 렌더링된 후 실행 | |
| setTimeout(setupInitialFocus, 0); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| }; | |
| } else if (previousFocusRef.current) { | |
| // 모달이 닫힐 때 이전 포커스 요소로 복귀 | |
| previousFocusRef.current.focus(); | |
| } | |
| }, [open, handleKeyDown, setupInitialFocus]); | |
| return contentRef; | |
| } | |
| export default useFocusTrap; | |
| useEffect(() => { | |
| if (open) { | |
| // 모달이 열리기 전 현재 포커스된 요소 저장 | |
| previousFocusRef.current = document.activeElement as HTMLElement; | |
| // 키보드 이벤트 리스너 등록 | |
| document.addEventListener('keydown', handleKeyDown); | |
| // 첫 번째 요소에 포커스 - 약간의 지연을 두어 DOM이 완전히 렌더링된 후 실행 | |
| - setTimeout(setupInitialFocus, 0); | |
| + const timeoutId = setTimeout(setupInitialFocus, 0); | |
| return () => { | |
| document.removeEventListener('keydown', handleKeyDown); | |
| + clearTimeout(timeoutId); | |
| }; | |
| } else if (previousFocusRef.current) { | |
| // 모달이 닫힐 때 이전 포커스 요소로 복귀 | |
| previousFocusRef.current.focus(); | |
| } | |
| }, [open, handleKeyDown, setupInitialFocus]); |
🤖 Prompt for AI Agents (early access)
In components/src/lib/components/Modal/hooks/useFocusTrap.ts around lines 52 and
66, first add a check before setting tabindex to '-1' on contentRef.current to
ensure it does not already have a tabindex attribute, preserving existing
values. Second, for the setTimeout call on line 66 used to delay initial focus
setup, store the timeout ID and clear it in a cleanup function returned by the
useEffect to prevent the timeout from running after the component unmounts.
| const StyledButton = styled.div` | ||
| background-color: inherit; | ||
| border: none; | ||
| `; |
There was a problem hiding this comment.
접근성 문제: StyledButton이 div로 정의되어 있습니다.
StyledButton이 div 요소로 정의되어 있어 접근성 문제가 발생할 수 있습니다. 이 컴포넌트가 상호작용 요소로 사용된다면, 실제 button 요소를 기반으로 하거나 적절한 ARIA 속성과 키보드 핸들링을 추가해야 합니다.
-const StyledButton = styled.div`
+const StyledButton = styled.button`
background-color: inherit;
border: none;
+ cursor: pointer;
`;📝 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 StyledButton = styled.div` | |
| background-color: inherit; | |
| border: none; | |
| `; | |
| const StyledButton = styled.button` | |
| background-color: inherit; | |
| border: none; | |
| cursor: pointer; | |
| `; |
🤖 Prompt for AI Agents (early access)
In components/src/lib/components/Modal/Modal.styles.ts around lines 40 to 43,
StyledButton is defined as a div, which causes accessibility issues for
interactive elements. Change StyledButton to be styled from a button element
instead of a div to ensure proper semantics and keyboard accessibility. If
additional ARIA attributes or keyboard event handlers are needed, add them
accordingly to maintain accessibility standards.
| import { CloseIcon } from './CloseIcon'; | ||
| import useModalContext, { ModalContext } from './hooks/useModalContext'; | ||
| import useFocusTrap from './hooks/useFocusTrap'; | ||
| import Button from '../Button/Button'; |
There was a problem hiding this comment.
Button import 방식 수정이 필요합니다.
Button 컴포넌트는 파일 내에서 default export가 아닌 named export로 선언되어 있는 것으로 보입니다. 하지만 현재 코드에서는 default import로 사용하고 있습니다.
-import Button from '../Button/Button';
+import { Button } from '../Button/Button';import 문법을 일관되게 유지하는 것이 좋습니다. Button 컴포넌트가 실제로 어떻게 export되는지 확인하고 그에 맞게 import 문을 수정해주세요.
📝 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.
| import Button from '../Button/Button'; | |
| -import Button from '../Button/Button'; | |
| +import { Button } from '../Button/Button'; |
🤖 Prompt for AI Agents (early access)
In components/src/lib/components/Modal/Modal.tsx at line 17, the Button
component is imported as a default export, but it is actually exported as a
named export. To fix this, change the import statement to use named import
syntax by enclosing Button in curly braces. Verify the export style in the
Button component file and update the import accordingly to maintain consistent
import/export usage.
|
|
||
| function Content({ children, className }: BaseProps) { | ||
| const { open, setOpen, size } = useModalContext(); | ||
| const contentRef = useFocusTrap(open, setOpen); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
useFocusTrap 훅 사용 방식 개선 필요
현재 useFocusTrap 훅에 open 상태와 setOpen 함수를 모두 전달하고 있습니다. 일반적으로 훅은 단일 책임 원칙에 따라 설계되는 것이 좋습니다. focus trap 기능은 모달의 열림/닫힘 제어와는 별개의 책임을 가져야 합니다.
focus trap 구현은 모달이 열려있는지 여부만 알면 되고, 모달을 닫는 기능은 별도로 처리하는 것이 바람직합니다.
-const contentRef = useFocusTrap(open, setOpen);
+const contentRef = useFocusTrap(open);그리고 ESC 키 이벤트 처리는 useFocusTrap이 아닌 Content 컴포넌트 내부에서 직접 처리하는 것을 고려해보세요.
📝 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 contentRef = useFocusTrap(open, setOpen); | |
| const contentRef = useFocusTrap(open); |
🤖 Prompt for AI Agents (early access)
In components/src/lib/components/Modal/Modal.tsx at line 58, the useFocusTrap
hook currently receives both the open state and setOpen function, mixing focus
management with modal open/close control. Refactor useFocusTrap to accept only
the open state to manage focus trapping, and move the ESC key event handling
logic out of useFocusTrap into the Content component itself for clearer
separation of concerns.
| function Content({ children, className }: BaseProps) { | ||
| const { open, setOpen, size } = useModalContext(); | ||
| const contentRef = useFocusTrap(open, setOpen); | ||
|
|
||
| if (!open) return null; | ||
|
|
||
| return ( | ||
| <> | ||
| <ModalOverlay onClick={() => setOpen(false)} /> | ||
| <ModalContentContainer | ||
| ref={contentRef} | ||
| className={className} | ||
| size={size || 'medium'} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| > | ||
| {children} | ||
| </ModalContentContainer> | ||
| </> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
키보드 접근성 개선을 위한 ESC 키 이벤트 핸들러 추가
현재 코드에서는 ESC 키를 눌러 모달을 닫는 기능이 구현되어 있지 않거나, useFocusTrap 훅에 위임되어 있습니다. 모달의 접근성을 높이기 위해 Content 컴포넌트에 명시적으로 ESC 키 이벤트 핸들러를 추가하는 것이 좋습니다.
function Content({ children, className }: BaseProps) {
const { open, setOpen, size } = useModalContext();
const contentRef = useFocusTrap(open, setOpen);
+ React.useEffect(() => {
+ const handleKeyDown = (event: KeyboardEvent) => {
+ if (event.key === 'Escape' && open) {
+ setOpen(false);
+ }
+ };
+
+ window.addEventListener('keydown', handleKeyDown);
+ return () => window.removeEventListener('keydown', handleKeyDown);
+ }, [open, setOpen]);
if (!open) return null;
return (
<>
<ModalOverlay onClick={() => setOpen(false)} />
<ModalContentContainer
ref={contentRef}
className={className}
size={size || 'medium'}
role="dialog"
aria-modal="true"
>
{children}
</ModalContentContainer>
</>
);
}이 변경은 사용자가 키보드로 모달을 쉽게 닫을 수 있게 하여 웹 접근성 표준(WCAG 2.1 2.1.2 키보드 트랩 없음)을 준수하는 데 도움이 됩니다.
🤖 Prompt for AI Agents (early access)
In components/src/lib/components/Modal/Modal.tsx around lines 56 to 76, the
Content component lacks an explicit ESC key event handler to close the modal,
which is important for keyboard accessibility. Add a keydown event listener
inside the Content component that listens for the ESC key and calls
setOpen(false) to close the modal. Make sure to add the event listener when the
modal opens and clean it up when it closes or the component unmounts to avoid
memory leaks.
bassyu
left a comment
bassyu
left a comment
There was a problem hiding this comment.
안녕하세요 메리!
2단계도 잘 부탁드립니다 🙇
라이브러리 방향성에 대해서 고민해주신 부분 좋네요 👍 👍
방향성을 토대로 추가 개선사항이나 질문 답변 코멘트 드렸습니다~!
코멘트 확인 후 다시 요청해주세요~!
| function Modal({ children, className, size = 'medium' }: ModalProps) { | ||
| const [open, setOpen] = useState(false); |
There was a problem hiding this comment.
상태를 외부에서 관리해주는 것 보다 내부적으로 관리하는 것이 사용자 입장에서는 보일러플레이트가 더 적기 때문에 편하지 않을까?
사용자 입장에서 고민 해주셨군요! 👍 동의합니다~!
다만 Trigger 버튼 클릭 말고도, 다른 방법으로 모달을 열고 싶어질 확률이 높습니다!
편리한 기능과 제어 컴포넌트 둘다 가능하도록 할 수 있을까요?
There was a problem hiding this comment.
쵸파의 리뷰를 보고 편리한 기능과 제어 컴포넌트 2가지의 기능을 모두 제공하는 방법을 생각해보았습니다.
제어 컴포넌트까지 제공하려면, 사용자가 모달을 사용하는 곳에서 모달의 열림/닫힘 상태를 관리하고 제어가 가능해야하는데, 이를 제공하려면 현재 Modal 컴포넌트를 수정해서 외부에서 제어할 수 있는 옵션을 추가해야 한다는 생각이 들었습니다.
이를 위해서는 일단 isOpen이라는 상태와 onOpenChange라는 props를 외부에서 받아 상태를 제어할 수 있도록 할 수 있게 해야 할 것 같습니다.
예를 들면 아래와 같이요!
const [isOpen, setIsOpen] = useState(false);
<Modal isOpen={isOpen} onOpenChange={setIsOpen}>
<Modal.Content>...</Modal.Content>
</Modal>
이렇게 되면 사용자 입장에서는 현재와 같은 Trigger 컴포넌트로 내부적인 상태를 사용하는 방식이 아닌 isOpen이라는 외부의 상태를 사용하는 방식으로도 모달을 사용할 수 있을 것 같습니다.
There was a problem hiding this comment.
넵 좋습니다! 👍 👍
Context를 사용하면서도, 충분히 초기값을 받거나 제어 컴포넌트로 활용할 수 있을 것 같아요~!
There was a problem hiding this comment.
이를 하나의 컴포넌트로 래핑한 AlertModal이라는 별도의 컴포넌트를 사용자에게 제공할 필요가 있을까요?
메리도 말씀 주셨듯이, 귀찮은 보일러플레이트나 구현 리소스를 줄이는 것이 라이브러리를 사용하는 이유중 하나라고 생각합니다~!
그래서 자유롭게 커스텀 할 수 있도록 분리된 컴포넌트도 제공하고,
AlertModal처럼 어느정도 제한된 규칙과 기능이 있는 완성본을 주는 것도 유용하다고 생각해요!
자유로운 컴포넌트 제공에만 집중하는 것이 컨셉이라면 이대로도 괜찮겠네요~!
There was a problem hiding this comment.
넵, 맞습니다!
최대한 사용자에게 일관된 사용방식을 주고 싶다는 의도로 구현하게 되었습니다.
합성 컴포넌트 패턴을 통해 모달의 기본 구성 요소는 제공하면서도 사용자가 각각의 필요에 맞게 유연하게 조합할 수 있는 선택지를 제공하고자 했습니다!
| <Modal> | ||
| <Modal.Trigger> | ||
| <Button>기본 모달 열기</Button> | ||
| </Modal.Trigger> | ||
| <Modal.Content> | ||
| <Modal.Header> | ||
| <div> | ||
| <Modal.Title>기본 모달</Modal.Title> | ||
| <Modal.Description>기본 모달 컴포넌트 설명</Modal.Description> | ||
| </div> | ||
| <Modal.CloseButton /> | ||
| </Modal.Header> | ||
| <div> | ||
| <p>기본 모달 내용</p> | ||
| </div> | ||
| </Modal.Content> | ||
| </Modal> |
There was a problem hiding this comment.
상태를 직접 정의해주긴 싫지만, 그 상태를 활용하고 싶을 수도 있는데요,
그럴 땐 아래처럼 renderProps를 활용하면 유용합니다!
| <Modal> | |
| <Modal.Trigger> | |
| <Button>기본 모달 열기</Button> | |
| </Modal.Trigger> | |
| <Modal.Content> | |
| <Modal.Header> | |
| <div> | |
| <Modal.Title>기본 모달</Modal.Title> | |
| <Modal.Description>기본 모달 컴포넌트 설명</Modal.Description> | |
| </div> | |
| <Modal.CloseButton /> | |
| </Modal.Header> | |
| <div> | |
| <p>기본 모달 내용</p> | |
| </div> | |
| </Modal.Content> | |
| </Modal> | |
| <Modal> | |
| {({ isOpen }) => | |
| <> | |
| <Modal.Trigger> | |
| <Button>{isOpen ? '이미 열림' : '기본 모달 열기'}</Button> | |
| </Modal.Trigger> | |
| <Modal.Content> | |
| <Modal.Header> | |
| <div> | |
| <Modal.Title>기본 모달</Modal.Title> | |
| <Modal.Description>기본 모달 컴포넌트 설명</Modal.Description> | |
| </div> | |
| <Modal.CloseButton /> | |
| </Modal.Header> | |
| <div> | |
| <p>기본 모달 내용</p> | |
| </div> | |
| </Modal.Content> | |
| </> | |
| </Modal> |
There was a problem hiding this comment.
이런 방식으로도 구현할 수 있네요 ,,
쵸파의 리뷰 덕분에 렌더 프롭 패턴에 대해서 알 수 있었습니다. 👍👍
렌더 프롭 패턴은 컴포넌트 내부에 있는 isOpen이라는 상태를 사용자가 사용하고 싶을 때 사용되는군요! 직접 써보지는 않았지만, 모달의 열림/닫힘 상태에 따라 사용자가 조건부 렌더링을 한다던지, 다른 스타일을 적용시키고 싶을 때 유용할 것 같아요.
| const cardNumber = useMemo(() => { | ||
| let result = ''; | ||
|
|
||
| if (!isNumber && value !== '') { | ||
| setCardNumberError({ | ||
| ...updatedError, | ||
| [name]: CARD_NUMBER_ERROR.onlyNumbers, | ||
| }); | ||
| for (let i = 0; i < requiredFields; i++) { | ||
| result += formattedCardNumber[i] || ''; | ||
| } | ||
| return result; | ||
| }, [formattedCardNumber, requiredFields]); |
There was a problem hiding this comment.
사용자 입력을 메모이제이션 해주셨네요~!
어떤 장단점이 있을까요?
이번 학습 주제는 아니어서 천천히 고민해주셔도 좋습니다!
There was a problem hiding this comment.
useMemo를 통해 사용자가 입력한 cardNumber를 메모이제이션 한 이유는 formattedCardNumber이나 requiredFields가 변경될 때만 연산을 수행하고, 그 외에는 불필요한 연산을 수행하지 않기 위해서 사용했습니다!
쵸파가 언급하신 장단점의 의미는 useMemo를 사용한 것에 대한 장단점을 의도하는게 맞을까요?
There was a problem hiding this comment.
쵸파가 언급하신 장단점의 의미는 useMemo를 사용한 것에 대한 장단점을 의도하는게 맞을까요?
넵 장단점이 있어서 무조건 사용하는 것이 좋지는 않다고 생각했어요~!
useMemo는 불필요한 연산을 줄이는 대신, 그 대가로 더 많은 메모리를 사용하게 되는데요,
사용자의 입력은 계속 변경되고, 입력의 가짓수가 너무 많아서 메모리 사용에 비해서 효율이 있을지 의문이 있었습니다!
앞으로도 어떤 상황에서 데이터를 저장할지 천천히 고민해봐도 좋을 것 같아요!
There was a problem hiding this comment.
따라서 해당 훅이 단일 책임 원칙에서 벗어나 있다는 생각이 드는데요.
이를 별도의 커스텀 훅으로 분리하는게 좋을 지 현재 구조를 유지하는 것이 더 나을지 쵸파의 생각이 궁금합니다!
서로 관련된 많은 기능을 제공하는 것이 목적이라면, 이대로도 괜찮습니다!
기능을 자연스럽게 연결하는 것이 useCardNumber의 역할이라고 봐도 될 것 같아요.
| return cardNumber.length === expectedTotalLength; | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
상태 흐름이 조금 복잡한데, 리팩터링 해보면 좋을 것 같습니다!
상태: ['232', '3213', '3123'] -> 파생 데이터: '32232131'
이렇게 상태를 활용해주셨는데요, 카드 브랜드마다 번호를 나누는 기준이 달라서 useEffect를 통해 길이를 잘라주셨네요!
상태: '32232131' -> 파생 데이터: ['232', '3213', '3123']
이런식으로 흐름을 바꾸어서 useEffect를 제거해볼 수 있을까요?
There was a problem hiding this comment.
우선 제가 처음에 상태를 배열로 관리한 이유는 외부 사용자가 데이터를 관리할 때 문자열이 담긴 배열으로 관리하지 않을까? 라고 생각했고, 그렇게 되면 현재 useCardNumber이라는 커스텀 훅을 사용할 때도 외부에서 관리하는 문자열 배열을 별도의 가공없이 넘겨주는 것이 사용자 입장에서도 편하다고 생각했기 때문에 현재의 방식으로 설계를 하게 되었습니다.
제가 생각하기에는 현재 구조의 문제점을 파악하지 못했었는데, 쵸파의 리뷰를 보고 현재 구조에는 어떤 문제가 있는지? 와 상태와 파생 데이터를 바꾸게 되면 어떤 장점이 있는 것인지를 생각해 보았습니다.
우선 현재 구조에는 어떤 문제가 있는지를 생각했을 때,
useEffect를 복잡하게 사용해서 디버깅을 어렵게 한다는 것입니다.
그리고 최근 수업을 통해 SSOT라는 개념을 알게되어, 제가 작성한 코드가 SSOT 원칙을 잘 지키고 있는 것인가 에 대해서도 생각을 해보게 되었습니다. 그 결과 엄밀히 따지면 상태의 순환적인 의존 때문에 SSOT 원칙을 완전히 지키고 있지 않는 것 같다.는 결론이 나왔습니다.
그 이유는 현재 상태 관리 방식을 보면
formattedCardNumber 배열이 주요 상태를 관리하고, 이 배열에서 cardNumber 문자열이 파생되는데, 이와 동시에 cardBrand가 formattedCardNumber에 의존해서 결정되고, 이 cardBrand에 따라 다시 formattedCardNumber 배열이 수정되기 때문입니다.
즉, 아래와 같은 데이터 흐름이 발생한다는 것을 파악할 수 있었습니다.
formattedCardNumber[0] → cardBrand → useEffect → formattedCardNumber
이렇게 순환적으로 의존하는 것은 SSOT 원칙을 위배하는 것이라는 생각이 들었습니다.
이런 관점으로 봤을 때, 쵸파의 리뷰에서 언급한 것 처럼 cardNumber 문자열으로 관리하게 되었을 때는 아래와 같은 단방향 데이터 흐름을 만들 수 있다고 판단했고, SSOT 원칙을 지킬 수 있을 것 같다고 생각했습니다.
cardNumber → cardBrand
cardNumber → formattedCardNumber
따라서 cardNumber를 기본 상태로 하여 문자열으로 관리하도록 하고, 이로 부터 파생된 데이터를 문자열 배열로 관리하는 구조가 SSOT 원칙을 위배하지 않으면서, useEffect를 제거할 수 있기 때문에 코드를 읽기 쉽다는 장점이 있다는 생각이 들었습니다.
쵸파가 언급하신 것처럼 상태를 문자열로 두고, 나머지 결과들을 파생 데이터로 관리하는 방식으로 수정하도록 하겠습니다. 감사합니다!
There was a problem hiding this comment.
There was a problem hiding this comment.
정리해주신 것처럼, state를 변화시키는 effect는 특히 주의해서 사용해야 합니다!
엄청 잘 정리 해주셨네요! 👍 👍
There was a problem hiding this comment.
넵 최대한 공통 컴포넌트를 뽑아서 재사용성을 높이고, 관심사를 분리하고자 했습니다!
There was a problem hiding this comment.
불필요한 props drilling이 발생해서 추후 이를 Context API를 도입하여 open 상태를 관리해주게 되었습니다.
개인적으로 모달은 얕은 편이라고 생각했는데요!
추후에는 props drilling을 render props나 children을 통해 해결하는 방법도 같이 고려해보면 좋을 것 같습니다~!
There was a problem hiding this comment.
세심한 리뷰 감사합니다!
저는 기존에 props drilling의 문제를 해소하는 방안 중에서 Context API가 있다는 것을 알고 있었기 때문에 해당 지식을 이번 미션에서 적용해 보았었는데요.
쵸파의 리뷰를 보고 Context API뿐만 아니라 children으로 해결하는 방법이 있다는 것도 알게 되었습니다!
감사합니다
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hooks/src/lib/useCardNumber/useCardNumber.ts (1)
85-88:⚠️ Potential issue정규식 검증 로직에 전체 문자열 매칭이 필요합니다
formatPattern.test(cardNumber)는 부분 문자열만 일치해도true를 반환할 수 있습니다. 정규식 패턴에 시작(^)과 끝($) 앵커를 추가하거나, 새 정규식을 생성하여 전체 문자열이 일치하는지 확인해야 합니다.다음과 같이 수정하는 것을 권장합니다:
-const formatPattern = CARD_RULES[cardBrand].formatPattern; -if (!formatPattern.test(cardNumber)) { +const formatPattern = CARD_RULES[cardBrand].formatPattern; +if (!new RegExp(`^${formatPattern.source}$`).test(cardNumber)) { return false; }또는
cardRules.ts에서 각formatPattern에^와$를 포함시키는 방법도 있습니다.
🧹 Nitpick comments (1)
hooks/src/lib/useCardNumber/useCardNumber.ts (1)
1-106: 단일 책임 원칙에 대한 고려가 필요합니다현재 구현은 카드 번호 검증, 포맷팅, 브랜드 감지, 오류 처리 등 여러 기능을 하나의 훅에서 처리하고 있습니다. 이러한 관심사를 분리하는 것을 고려해 볼 수 있습니다.
예를 들어:
useCardValidation- 카드 번호 유효성 검사만 담당useCardFormatting- 카드 번호 포맷팅만 담당useCardBrandDetection- 카드 브랜드 감지만 담당이런 방식으로 분리하면 각 훅의 책임이 명확해지고, 필요한 기능만 조합하여 사용할 수 있게 됩니다.
하지만 현재처럼 관련 기능을 하나의 훅으로 제공하는 것도 사용자 편의성 측면에서는 장점이 있습니다. 서로 관련된 많은 기능을 제공하는 것이 목적이라면, 현재 구조도 충분히 좋은 접근 방식입니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (2)
hooks/package.json(1 hunks)hooks/src/lib/useCardNumber/useCardNumber.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/package.json
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: You MUST follow these guidelines: - Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, re...
**/*.ts: You MUST follow these guidelines:
- Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, reference official documentation that can help resolve these issues
- Analyze the commit history to infer the cognitive process the developer went through while solving the problem. Summarize their thought progression: how they understood the problem, iterated on solutions, and refined the code. Focus on their reasoning patterns, learning moments, or shifts in approach. If applicable, relate their process to established best practices in the React ecosystem.
- Do not provide complete solutions unless explicitly requested.
- Guide the programmer in problem-solving instead of providing direct answers.
- When asked about programming concepts (e.g., "What is a hook?"), give direct and clear explanations.
- Break problems into smaller, manageable steps to help think through them logically.
- Ask leading questions and provide hints instead of just telling the answer.
- Encourage independent debugging before offering suggestions.
- Refer to relevant documentation instead of providing ready-made solutions.
- Promote modular thinking—breaking problems into reusable components.
- Remind the programmer to reflect on lessons learned after solving an issue.
- If explicitly asked for code (e.g., "Give me the code"), then provide it.
hooks/src/lib/useCardNumber/useCardNumber.ts
🔇 Additional comments (6)
hooks/src/lib/useCardNumber/useCardNumber.ts (6)
7-10: 상수 정의가 명확합니다
CARD_NUMBER상수를 통해 최대 길이와 필드 수를 명시적으로 정의한 점이 좋습니다. 이렇게 매직 넘버를 피하고 상수로 관리하면 코드의 가독성과 유지보수성이 높아집니다.
12-34: splitCardNumber 함수 구현이 깔끔합니다카드 번호를 브랜드별 필드 길이에 따라 분할하는 로직이 잘 구현되어 있습니다. 빈 문자열 처리, 브랜드 식별, 그리고 필드 길이에 따른 분할 로직이 명확합니다.
36-39: 상태 관리 방식의 개선이 눈에 띕니다이전 피드백을 반영하여 카드 번호를 문자열로 관리하도록 변경한 점이 좋습니다. 단일 진실 공급원(SSOT) 원칙을 따르는 방향으로 개선되었습니다.
40-46: useMemo 사용이 적절합니다
cardBrand,fieldLengthArr,requiredFields,formattedCardNumber를 메모이제이션하여 성능을 최적화했습니다. 의존성 배열도 올바르게 설정되어 있습니다.
47-73: handleCardNumberChange 함수의 검증 로직이 잘 구현되어 있습니다입력값 검증, 에러 처리, 최대 길이 제한 등의 로직이 잘 구현되어 있습니다. 특히 카드 브랜드에 따라 동적으로 필드 길이를 적용하는 부분이 좋습니다.
93-102: 반환 객체에 추가 메타데이터 제공이 좋습니다
cardBrand,requiredFields,fieldLengthArr와 같은 추가 메타데이터를 반환하여 훅 사용자에게 더 많은 정보와 유연성을 제공하는 점이 좋습니다.
|
안녕하세요 쵸파 앞으로도 이번 미션에서 얻은 인사이트를 활용할 수 있을 것 같습니다! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hooks/src/lib/useCardNumber/useCardNumber.test.ts (1)
1-119: 오류 검증 테스트 확장이 필요할 수 있습니다.현재 테스트는 숫자가 아닌 값 입력에 대한 오류 검증은 하고 있지만, 카드 브랜드별 유효성 검사 규칙(예: Luhn 알고리즘, 특정 브랜드별 체크섬 등)에 대한 테스트는 부족해 보입니다. 브랜드별 검증 로직이 구현되어 있다면 이에 대한 테스트도 추가하는 것이 좋겠습니다.
다음과 같은 카드 브랜드별 유효성 검사 테스트를 추가하는 것을 고려해보세요:
it('Luhn 알고리즘을 통한 카드 번호 검증이 올바르게 수행되어야 한다', () => { // 유효한 VISA 카드 번호 (Luhn 알고리즘 통과) const { result: validResult } = renderHook(() => useCardNumber('4111111111111111', emptyErrorMsg) ); expect(validResult.current.isCardNumberValid()).toBe(true); // 유효하지 않은 VISA 카드 번호 (Luhn 알고리즘 실패) const { result: invalidResult } = renderHook(() => useCardNumber('4111111111111112', emptyErrorMsg) ); expect(invalidResult.current.isCardNumberValid()).toBe(false); }); it('브랜드별 카드 번호 길이 제한이 올바르게 검증되어야 한다', () => { // VISA는 13자리 또는 16자리가 유효함 const { result: validVisa16 } = renderHook(() => useCardNumber('4111111111111111', emptyErrorMsg) ); expect(validVisa16.current.isCardNumberValid()).toBe(true); const { result: validVisa13 } = renderHook(() => useCardNumber('4111111111111', emptyErrorMsg) ); expect(validVisa13.current.isCardNumberValid()).toBe(true); // AMEX는 15자리만 유효함 const { result: invalidAmex } = renderHook(() => useCardNumber('371449635398', emptyErrorMsg) ); expect(invalidAmex.current.isCardNumberValid()).toBe(false); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Knowledge Base: Disabled due to Reviews > Disable Knowledge Base setting
📒 Files selected for processing (1)
hooks/src/lib/useCardNumber/useCardNumber.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: You MUST follow these guidelines: - Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, re...
**/*.ts: You MUST follow these guidelines:
- Please leave 1–2 key review comments that address recurring mistakes or pain points the code author frequently makes. If possible, reference official documentation that can help resolve these issues
- Analyze the commit history to infer the cognitive process the developer went through while solving the problem. Summarize their thought progression: how they understood the problem, iterated on solutions, and refined the code. Focus on their reasoning patterns, learning moments, or shifts in approach. If applicable, relate their process to established best practices in the React ecosystem.
- Do not provide complete solutions unless explicitly requested.
- Guide the programmer in problem-solving instead of providing direct answers.
- When asked about programming concepts (e.g., "What is a hook?"), give direct and clear explanations.
- Break problems into smaller, manageable steps to help think through them logically.
- Ask leading questions and provide hints instead of just telling the answer.
- Encourage independent debugging before offering suggestions.
- Refer to relevant documentation instead of providing ready-made solutions.
- Promote modular thinking—breaking problems into reusable components.
- Remind the programmer to reflect on lessons learned after solving an issue.
- If explicitly asked for code (e.g., "Give me the code"), then provide it.
hooks/src/lib/useCardNumber/useCardNumber.test.ts
🔇 Additional comments (6)
hooks/src/lib/useCardNumber/useCardNumber.test.ts (6)
6-8: 테스트에 사용할 상수 변수들이 명확하게 정의되었습니다.상수 변수들을 별도로 정의하여 테스트 코드의 가독성과 유지보수성을 높였습니다. 이렇게 하면 테스트 값을 한 곳에서 관리할 수 있어 테스트 시나리오 변경이 필요할 때 용이합니다.
13-15: 초기값 테스트가 세분화되었습니다.cardNumber, formattedCardNumber, cardNumberError의 초기값을 개별적으로 확인하는 방식으로 테스트가 개선되었습니다. 배열 기반 데이터 구조로 변경된 내부 상태를 정확하게 검증합니다.
18-28: 카드 번호 입력 처리 로직의 테스트가 개선되었습니다.입력 값 처리 테스트에서 name 대신 idx를 사용하도록 변경되었고, 배열 기반 상태 업데이트를 검증하는 방식으로 개선되었습니다. 또한 최종 카드 번호 문자열 상태도 확인하는 테스트가 추가되었습니다.
68-80: 카드 브랜드 감지 기능을 위한 테스트 케이스가 추가되었습니다.VISA, MASTERCARD, AMEX, UNKNOWN 브랜드를 정확히 감지하는지 검증하는 테스트 케이스가 추가되었습니다. 다양한 카드 브랜드에 대한 테스트가 포함되어 강건성이 향상되었습니다.
82-92: 카드 브랜드별 필드 구성 테스트가 추가되었습니다.각 카드 브랜드(VISA, AMEX)에 따라 필드 수와 길이가 올바르게 설정되는지 검증하는 테스트가 추가되었습니다. 이는 브랜드별 특성을 반영한 동적 UI 구성을 지원하는 중요한 기능입니다.
94-118: 카드 브랜드 변경에 따른 필드 동적 구성 검증 테스트가 추가되었습니다.사용자 입력에 따라 카드 브랜드가 변경될 때 필드 구성(필드 수와 각 필드 길이)이 올바르게 동적으로 업데이트되는지 검증하는 테스트가 추가되었습니다. 이 테스트는 실제 사용자 경험을 시뮬레이션하는 중요한 시나리오를 다루고 있습니다.
| import { renderHook, act } from '@testing-library/react'; | ||
| import { useCardNumber } from './useCardNumber'; | ||
| import { CARD_NUMBER_ERROR } from '../constants/errorMessages'; | ||
| import { CardNumber, CardNumberError } from '../types/cardTypes'; | ||
|
|
||
| describe('useCardNumber 훅 테스트', () => { | ||
| const emptyCardNumber: CardNumber = { | ||
| first: '', | ||
| second: '', | ||
| third: '', | ||
| forth: '', | ||
| }; | ||
|
|
||
| const validCardNumber: CardNumber = { | ||
| first: '1234', | ||
| second: '5678', | ||
| third: '9012', | ||
| forth: '3456', | ||
| }; | ||
|
|
||
| const emptyCardNumberError: CardNumberError = { | ||
| first: '', | ||
| second: '', | ||
| third: '', | ||
| forth: '', | ||
| }; | ||
| const emptyCardNumber = ''; | ||
| const validCardNumber = '1234567890123456'; | ||
| const emptyErrorMsg = ''; | ||
|
|
||
| it('초기값이 올바르게 설정되어야 한다', () => { | ||
| const { result } = renderHook(() => useCardNumber(validCardNumber, emptyCardNumberError)); | ||
| const { result } = renderHook(() => useCardNumber(validCardNumber, emptyErrorMsg)); | ||
|
|
||
| expect(result.current.cardNumber).toEqual(validCardNumber); | ||
| expect(result.current.cardNumberError).toEqual(emptyCardNumberError); | ||
| expect(result.current.cardNumber).toBe(validCardNumber); | ||
| expect(result.current.formattedCardNumber).toEqual(['1234', '5678', '9012', '3456']); | ||
| expect(result.current.cardNumberError).toEqual(['', '', '', '']); | ||
| }); | ||
|
|
||
| it('숫자가 입력되면 cardNumber가 업데이트되고 에러 메시지가 지워져야 한다', () => { | ||
| const { result } = renderHook(() => useCardNumber(emptyCardNumber, emptyCardNumberError)); | ||
| it('숫자가 입력되면 formattedCardNumber가 업데이트되고 에러 메시지가 지워져야 한다', () => { | ||
| const { result } = renderHook(() => useCardNumber(emptyCardNumber, emptyErrorMsg)); | ||
|
|
||
| act(() => { | ||
| result.current.handleCardNumberChange({ name: 'first', value: '1234' }); | ||
| result.current.handleCardNumberChange({ idx: 0, value: '1234' }); | ||
| }); | ||
|
|
||
| expect(result.current.cardNumber.first).toBe('1234'); | ||
| expect(result.current.cardNumberError.first).toBe(''); | ||
| expect(result.current.formattedCardNumber[0]).toBe('1234'); | ||
| expect(result.current.cardNumberError[0]).toBe(''); | ||
| expect(result.current.cardNumber).toBe('1234'); | ||
| }); | ||
|
|
||
| it('모든 필드가 유효한 길이이면 isCardNumberValid가 true를 반환해야 한다', () => { | ||
| const { result } = renderHook(() => useCardNumber(validCardNumber, emptyCardNumberError)); | ||
|
|
||
| const { result } = renderHook(() => useCardNumber(validCardNumber, emptyErrorMsg)); | ||
| expect(result.current.isCardNumberValid()).toBe(true); | ||
| }); | ||
|
|
||
| it('숫자가 아닌 값이 입력되면 에러 메시지가 설정되어야 한다', () => { | ||
| const { result } = renderHook(() => useCardNumber(emptyCardNumber, emptyCardNumberError)); | ||
| const { result } = renderHook(() => useCardNumber(emptyCardNumber, emptyErrorMsg)); | ||
|
|
||
| act(() => { | ||
| result.current.handleCardNumberChange({ name: 'first', value: 'abcd' }); | ||
| result.current.handleCardNumberChange({ idx: 0, value: 'abcd' }); | ||
| }); | ||
|
|
||
| expect(result.current.cardNumberError.first).toBe(CARD_NUMBER_ERROR.onlyNumbers); | ||
| expect(result.current.cardNumber.first).toBe(''); | ||
| expect(result.current.cardNumberError[0]).toBe(CARD_NUMBER_ERROR.onlyNumbers); | ||
| expect(result.current.formattedCardNumber[0]).toBe(''); | ||
| }); | ||
|
|
||
| it('빈 값이 입력되면 해당 필드가 빈 문자열로 설정되어야 한다', () => { | ||
| const initialCardNumber = { ...validCardNumber }; | ||
| const { result } = renderHook(() => useCardNumber(initialCardNumber, emptyCardNumberError)); | ||
| const { result } = renderHook(() => useCardNumber(validCardNumber, emptyErrorMsg)); | ||
|
|
||
| act(() => { | ||
| result.current.handleCardNumberChange({ name: 'first', value: '' }); | ||
| result.current.handleCardNumberChange({ idx: 0, value: '' }); | ||
| }); | ||
|
|
||
| expect(result.current.cardNumber.first).toBe(''); | ||
| expect(result.current.cardNumberError.first).toBe(''); | ||
| expect(result.current.cardNumberError[0]).toBe(''); | ||
| }); | ||
|
|
||
| it('한 필드가 유효하지 않으면 isCardNumberValid가 false를 반환해야 한다', () => { | ||
| const invalidCardNumber: CardNumber = { | ||
| ...validCardNumber, | ||
| first: '123', | ||
| }; | ||
|
|
||
| const { result } = renderHook(() => useCardNumber(invalidCardNumber, emptyCardNumberError)); | ||
| const { result } = renderHook(() => useCardNumber('123567890123456', emptyErrorMsg)); | ||
|
|
||
| expect(result.current.isCardNumberValid()).toBe(false); | ||
| }); | ||
|
|
||
| it('여러 필드가 유효하지 않으면 isCardNumberValid가 false를 반환해야 한다', () => { | ||
| const invalidCardNumber: CardNumber = { | ||
| first: '123', | ||
| second: '56', | ||
| third: '9012', | ||
| forth: '3456', | ||
| }; | ||
|
|
||
| const { result } = renderHook(() => useCardNumber(invalidCardNumber, emptyCardNumberError)); | ||
| const { result } = renderHook(() => useCardNumber('12356', emptyErrorMsg)); | ||
|
|
||
| expect(result.current.isCardNumberValid()).toBe(false); | ||
| }); | ||
|
|
||
| it('카드 브랜드가 올바르게 감지되어야 한다', () => { | ||
| const { result: visaResult } = renderHook(() => useCardNumber('4111111111111111', emptyErrorMsg)); | ||
| expect(visaResult.current.cardBrand).toBe('VISA'); | ||
|
|
||
| const { result: mastercardResult } = renderHook(() => useCardNumber('5555555555554444', emptyErrorMsg)); | ||
| expect(mastercardResult.current.cardBrand).toBe('MASTERCARD'); | ||
|
|
||
| const { result: amexResult } = renderHook(() => useCardNumber('371449635398431', emptyErrorMsg)); | ||
| expect(amexResult.current.cardBrand).toBe('AMEX'); | ||
|
|
||
| const { result: unknownResult } = renderHook(() => useCardNumber('1234567890123456', emptyErrorMsg)); | ||
| expect(unknownResult.current.cardBrand).toBe('UNKNOWN'); | ||
| }); | ||
|
|
||
| it('필드 수와 길이가 카드 브랜드에 따라 올바르게 설정되어야 한다', () => { | ||
| // VISA 카드 | ||
| const { result: visaResult } = renderHook(() => useCardNumber('4111111111111111', emptyErrorMsg)); | ||
| expect(visaResult.current.requiredFields).toBe(4); | ||
| expect(visaResult.current.fieldLengthArr).toEqual([4, 4, 4, 4]); | ||
|
|
||
| // AMEX 카드 | ||
| const { result: amexResult } = renderHook(() => useCardNumber('371449635398431', emptyErrorMsg)); | ||
| expect(amexResult.current.requiredFields).toBe(3); | ||
| expect(amexResult.current.fieldLengthArr).toEqual([4, 6, 5]); | ||
| }); | ||
|
|
||
| it('카드 브랜드가 변경되면 필드 구성이 변경되어야 한다', () => { | ||
| const { result } = renderHook(() => useCardNumber('', emptyErrorMsg)); | ||
|
|
||
| expect(result.current.cardBrand).toBe('UNKNOWN'); | ||
| expect(result.current.requiredFields).toBe(4); | ||
| expect(result.current.fieldLengthArr).toEqual([4, 4, 4, 4]); | ||
|
|
||
| act(() => { | ||
| result.current.handleCardNumberChange({ idx: 0, value: '4111' }); | ||
| }); | ||
|
|
||
| // VISA 브랜드로 변경 | ||
| expect(result.current.cardBrand).toBe('VISA'); | ||
| expect(result.current.requiredFields).toBe(4); | ||
| expect(result.current.fieldLengthArr).toEqual([4, 4, 4, 4]); | ||
|
|
||
| act(() => { | ||
| result.current.handleCardNumberChange({ idx: 0, value: '3714' }); | ||
| }); | ||
|
|
||
| // AMEX 브랜드로 변경 | ||
| expect(result.current.cardBrand).toBe('AMEX'); | ||
| expect(result.current.requiredFields).toBe(3); | ||
| expect(result.current.fieldLengthArr).toEqual([4, 6, 5]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
새로운 API(idx 기반)에 대한 문서화 및 마이그레이션 가이드가 필요합니다.
기존 name 키에서 idx 키로 변경된 API는 훅을 사용하는 개발자에게 영향을 줄 수 있습니다. 이 변경사항에 대한 마이그레이션 가이드나 문서를 제공하는 것이 좋겠습니다. 특히 API 설계 결정에 대한 근거와 사용 예시가 포함된 문서를 작성하면 사용자의 이해를 도울 수 있습니다.
README.md나 JSDoc 주석에 다음과 같은 내용을 추가하는 것을 고려해보세요:
## Breaking Changes in v2.0
### handleCardNumberChange API 변경
v1.x:
```typescript
handleCardNumberChange({ name: 'cardNumber1', value: '1234' })v2.x:
handleCardNumberChange({ idx: 0, value: '1234' })이 변경의 이유:
- 카드 브랜드별로 필드 수와 길이가 동적으로 변경되므로, 하드코딩된 name 대신 유연한 idx 기반 접근 방식을 채택했습니다.
- 이는 AMEX 카드와 같이 표준 4-4-4-4 형식을 따르지 않는 카드 브랜드를 지원하기 위함입니다.
<details>
<summary>🤖 Prompt for AI Agents</summary>
In hooks/src/lib/useCardNumber/useCardNumber.test.ts lines 1 to 119, the test
code uses a new API where the handleCardNumberChange function accepts an idx key
instead of the previous name key. To assist developers using this hook, add
documentation such as a README.md section or JSDoc comments explaining this
breaking change, including the old and new API usage examples, the rationale
behind switching from name to idx, and how this supports dynamic field lengths
for different card brands like AMEX. This will provide a clear migration guide
and improve user understanding.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
bassyu
left a comment
bassyu
left a comment
There was a problem hiding this comment.
안녕하세요 메리!
간단하게 코멘트 드렸는데도 많이 고민해주시고, 잘 정리해 주셨네요! 😮 👍
useMemo관련해서 천천히 고민해봐도 좋을 것 같아요!
2단계도 이만 머지하겠습니다!
수고 많으셨고 다음 미션도 파이팅입니다~! 💪 💪
| function Modal({ children, className, size = 'medium' }: ModalProps) { | ||
| const [open, setOpen] = useState(false); |
There was a problem hiding this comment.
넵 좋습니다! 👍 👍
Context를 사용하면서도, 충분히 초기값을 받거나 제어 컴포넌트로 활용할 수 있을 것 같아요~!
| const cardNumber = useMemo(() => { | ||
| let result = ''; | ||
|
|
||
| if (!isNumber && value !== '') { | ||
| setCardNumberError({ | ||
| ...updatedError, | ||
| [name]: CARD_NUMBER_ERROR.onlyNumbers, | ||
| }); | ||
| for (let i = 0; i < requiredFields; i++) { | ||
| result += formattedCardNumber[i] || ''; | ||
| } | ||
| return result; | ||
| }, [formattedCardNumber, requiredFields]); |
There was a problem hiding this comment.
쵸파가 언급하신 장단점의 의미는 useMemo를 사용한 것에 대한 장단점을 의도하는게 맞을까요?
넵 장단점이 있어서 무조건 사용하는 것이 좋지는 않다고 생각했어요~!
useMemo는 불필요한 연산을 줄이는 대신, 그 대가로 더 많은 메모리를 사용하게 되는데요,
사용자의 입력은 계속 변경되고, 입력의 가짓수가 너무 많아서 메모리 사용에 비해서 효율이 있을지 의문이 있었습니다!
앞으로도 어떤 상황에서 데이터를 저장할지 천천히 고민해봐도 좋을 것 같아요!
| return cardNumber.length === expectedTotalLength; | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
정리해주신 것처럼, state를 변화시키는 effect는 특히 주의해서 사용해야 합니다!
엄청 잘 정리 해주셨네요! 👍 👍
2 participants
안녕하세요 쵸파!☺️
일주일만에 또 뵙네요 ㅎㅅㅎ
1단계에 이어 2단계도 잘 부탁드립니다
🎯 페이먼츠 모듈
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계 - Module
2단계 - Refactoring
🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
🗂️ Modal
🗂️ Custom Hooks
🎨 스토리북 배포 주소
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
1️⃣ 합성 컴포넌트 도입
지난 리뷰를 받고 라이브러리의 방향성에 대해서 생각해봤습니다. 그러다 보니 현재 제가 구현한 모달은 외부 사용자를 고려한 것이 아니라 내부적인 모듈에 가깝다는 생각이 들었습니다.
현재 모달의 구조는 단일 컴포넌트였기 때문에 아래와 같은 문제점이 있다고 판단했습니다.
props가 늘어날 수록 컴포넌트가 비대해진다.따라서 "외부 사용자들이 다양한 상황에서 유연하게 사용할 수 있는 모달"을 만들고자 했고, 다양한 패턴이 있겠지만 그 중에서 합성 컴포넌트 패턴을 도입하게 되었습니다.
적용해보니 확실히 사용자의 자유도를 높여준다는 점에서 장점이 있다는 것을 체감할 수 있었습니다. 또한
Modal을 호출해서 사용하는 입장에서도 기존에는Modal컴포넌트 하나만 호출했다면, 합성 컴포넌트를 통해 계층 구조를 명확하게 볼 수 있기 때문에 전체 코드를 이해하는 과정에서도 어려움이 없다는 생각이 들었습니다.2️⃣
open state관리step1에서는 모달의 열림/닫힘 상태를 외부에서 관리해주었습니다. (부모 컴포넌트가 모달의 열림/닫힘 상태를 제어하는 형태)하지만 이렇게 상태를 외부에서 관리해주는 것 보다 내부적으로 관리하는 것이 사용자 입장에서는 보일러플레이트가 더 적기 때문에 편하지 않을까? 라는 생각이 들었습니다.
따라서 모달의 열림/닫힘 상태를 관리하는
open상태를Modal컴포넌트 내부에서 관리하게 되었습니다. 또한 합성 컴포넌트 방식을 사용하다 보니 불필요한props drilling이 발생해서 추후 이를Context API를 도입하여open상태를 관리해주게 되었습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
1️⃣ 다양한 타입의
Modal에 대한 단일 컴포넌트 제공 여부저는 사용자가
AlertModal이나ConfirmModal에 대한 예시 코드를 복사해서 필요한 부분만 변경해 사용한다고 가정했을 때, 합성 컴포넌트 방식만 제공해도 괜찮다고 생각했기 때문에 2단계 요구사항인AlertModal,ConfirmModal과 같은 별도의 컴포넌트는 구현하지 않았습니다. (대신Storybook은 작성해 주었습니다.) 이를 하나의 컴포넌트로 래핑한AlertModal이라는 별도의 컴포넌트를 사용자에게 제공할 필요가 있을까요?그렇다고 두 가지 옵션을 모두 제공하는 것은 라이브러리의 일관성이 떨어질 뿐더러 사용하는 사용자 입장에서 혼란을 일으킬 수 있다고 생각합니다.
합성 컴포넌트만 제공하는 것과 두 가지의 옵션 모두 제공하는 것 중 어떤 방향으로 가는 것이 라이브러리의 사용자 입장에서 편의성을 갖춘 라이브러리라고 생각하는지 쵸파의 의견이 궁금합니다!
2️⃣
useCardNumber분리의 필요성2단계 요구사항을 구현하면서, 어쩌다 보니 모든 로직을
useCardNumber커스텀 훅에 작성하게 되었습니다.따라서 현재
useCardNumber커스텀 훅에는 카드 번호 검증, 포맷팅, 카드 브랜드 식별, 에러 관리와 같은 여러 책임이 한 곳에 작성되어 있습니다.따라서 해당 훅이 단일 책임 원칙에서 벗어나 있다는 생각이 드는데요.
이를 별도의 커스텀 훅으로 분리하는게 좋을 지 현재 구조를 유지하는 것이 더 나을지 쵸파의 생각이 궁금합니다!
✅ 리뷰어 체크 포인트
1. 모달 컴포넌트의 유연성
2. 커스텀 훅의 역할과 모듈화
3. 모듈 배포