[1단계 - 페이먼츠] 라바(고제성) 미션 제출합니다.#522
Conversation
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
…입력 에러 메세지 반환 Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
Co-authored-by: yuncic <yuncic@users.noreply.github.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough이 변경사항은 React/Vite 기반 결제 폼 UI 애플리케이션의 완전한 프로젝트 초기 설정을 도입합니다. Vite, TypeScript, ESLint, Storybook 구성 파일부터 메인 애플리케이션 컴포넌트, 카드 미리보기, 입력 래퍼 컴포넌트(카드번호/유효기간/CVC)를 포함한 비즈니스 로직 코드, 결제 데이터 검증 유틸리티 함수, 패키지 의존성 선언 및 프로젝트 문서화를 포함합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App.tsx`:
- Around line 70-80: MainContainer is using position: fixed which prevents
scrolling and can push inputs/error messages off-screen; remove position: fixed
and the top/left/transform rules from the MainContainer styled component and
instead let it participate in normal document flow (e.g., use default/static or
relative positioning) and rely on flexbox/margins/padding or a parent container
to center and constrain the form so the page becomes vertically scrollable when
viewport is short; update the MainContainer styled definition accordingly and
ensure any layout that depended on those fixed coordinates (centering logic) is
replaced by responsive centering (e.g., margin:auto or a centered parent).
In `@src/components/Card/CardPreview.tsx`:
- Around line 17-18: The CardPreview rendering currently outputs CardBrandImage
without alt text; update the JSX where isMasterCardNumber(cardNumbers) and
isVisaCardNumber(cardNumbers) are used so each CardBrandImage includes a
descriptive alt (e.g., "Mastercard logo" and "Visa logo") and ensure the
CardBrandImage component/prop types accept an alt string (update its
props/interface if necessary) so screen readers can announce the brand.
In `@src/components/CardInfoSection.tsx`:
- Around line 35-41: The styled component CaptionStyle currently uses the HTML
<caption> element (CaptionStyle) which is table-only; replace its tag with a
semantically neutral inline/block element (e.g., styled.span or styled.p) and
update any usages of CaptionStyle in CardInfoSection to use the new component so
the markup becomes valid outside of a table; ensure styling and accessibility
remain the same (add an appropriate ARIA role only if needed).
In `@src/utils/getCardNumberErrorMessage.ts`:
- Around line 4-6: getCardNumberErrorMessage currently only checks each chunk's
length and can accept arrays with wrong number of chunks; update the function to
first assert the incoming array length is exactly 4 and then validate that every
element has length 4, returning ERROR_MESSAGE if either check fails and null
otherwise (referencing getCardNumberErrorMessage and ERROR_MESSAGE to locate the
logic).
In `@src/utils/getCVCNumberErrorMessage.ts`:
- Around line 3-5: The validator getCVCumberErrorMessage currently only checks
length and will accept non-digit strings like "12a"; update the function to
require exactly three numeric digits (e.g. validate with a regex for /^\d{3}$/
or by combining /^\d+$/ and length === 3) and return CVC_LENGTH_ERROR_MESSAGE
when that check fails; also fix the function name to match the file intent
(rename getCVCumberErrorMessage → getCVCNumberErrorMessage) so callers and the
file name are consistent.
In `@src/utils/isMasterCardNumber.ts`:
- Around line 1-5: The function isMasterCardNumber accesses cardNumbers[0]
without guarding against an empty array, causing an exception on initial render;
update isMasterCardNumber to first check that cardNumbers is an array with at
least one element and that cardNumbers[0] is a non-empty string (e.g., ensure
cardNumbers?.length > 0 and typeof cardNumbers[0] === 'string' and
cardNumbers[0].length >= 2) and return false early for empty/invalid input
before performing the existing digit checks (keep the existing checks using
cardNumbers[0][0] and cardNumbers[0][1]).
In `@src/utils/isMonthMatch.ts`:
- Around line 1-5: The isMonthMatch function currently checks length and numeric
range but allows non-numeric two-character strings; update isMonthMatch to first
ensure the input is a two-digit numeric string (e.g., validate with a digit-only
check or regex like /^\d{2}$/) before converting to Number and checking range
01–12, and keep the explicit length check only as part of that numeric-string
validation so non-digit values (which would produce NaN) are rejected.
In `@src/utils/isVisaCardNumber.ts`:
- Around line 1-6: The isVisaCardNumber helper currently assumes cardNumbers[0]
exists and returns undefined for non-Visa values; update isVisaCardNumber to
defensively handle empty arrays and non-string entries by first checking that
cardNumbers is an array with at least one element and that cardNumbers[0] is a
non-empty string, then return a boolean for all code paths (true when the first
character === '4', otherwise false). Ensure you reference the function name
isVisaCardNumber and adjust its early returns so it never throws on empty input
and always returns a boolean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0d58d33e-f501-4b02-bb9e-4817b4a5a2be
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpublic/Mastercard.svgis excluded by!**/*.svgpublic/Visa.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
.gitignore.storybook/main.ts.storybook/preview.tseslint.config.jsindex.htmlpackage.jsonsrc/App.tsxsrc/components/Card/CardPreview.stories.tsxsrc/components/Card/CardPreview.tsxsrc/components/CardInfoSection.tsxsrc/components/Input/CardInfoInput.stories.tsxsrc/components/Input/CardInfoInput.tsxsrc/components/InputWrapper/CVCInputWrapper.tsxsrc/components/InputWrapper/CardInputWrapper.tsxsrc/components/InputWrapper/CardNumberInputWrapper.tsxsrc/components/InputWrapper/EXPInputWrapper.tsxsrc/main.tsxsrc/utils/getCVCNumberErrorMessage.tssrc/utils/getCardNumberErrorMessage.tssrc/utils/getEXPNumberErrorMessage.tssrc/utils/isLengthMatch.tssrc/utils/isMasterCardNumber.tssrc/utils/isMonthMatch.tssrc/utils/isVisaCardNumber.tstsconfig.app.jsontsconfig.jsontsconfig.node.jsonvite.config.tsvitest.shims.d.ts기능목록.md
| const MainContainer = styled.main` | ||
| position: fixed; | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: center; | ||
| align-items: center; | ||
| gap: 45px; | ||
| top: 50%; | ||
| left: 50%; | ||
| transform: translate(-50%, -50%); | ||
| `; |
There was a problem hiding this comment.
position: fixed는 이 폼에 너무 강한 제약입니다.
화면 높이가 조금만 줄어도 입력이나 오류 메시지가 화면 밖으로 밀려서 접근이 어려워집니다. 결제 폼은 스크롤 가능한 일반 문서 흐름으로 두는지 확인해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.tsx` around lines 70 - 80, MainContainer is using position: fixed
which prevents scrolling and can push inputs/error messages off-screen; remove
position: fixed and the top/left/transform rules from the MainContainer styled
component and instead let it participate in normal document flow (e.g., use
default/static or relative positioning) and rely on flexbox/margins/padding or a
parent container to center and constrain the form so the page becomes vertically
scrollable when viewport is short; update the MainContainer styled definition
accordingly and ensure any layout that depended on those fixed coordinates
(centering logic) is replaced by responsive centering (e.g., margin:auto or a
centered parent).
| {isMasterCardNumber(cardNumbers) && <CardBrandImage src={Mastercard} />} | ||
| {isVisaCardNumber(cardNumbers) && <CardBrandImage src={Visa} />} |
There was a problem hiding this comment.
브랜드 로고에 대체 텍스트를 추가하세요.
현재 img가 설명 없이 렌더링되어 스크린리더가 의미를 읽지 못합니다. 브랜드 정보는 카드 프리뷰의 핵심이므로 alt를 명시하는 편이 안전합니다.
♿ 수정 예시
- {isMasterCardNumber(cardNumbers) && <CardBrandImage src={Mastercard} />}
- {isVisaCardNumber(cardNumbers) && <CardBrandImage src={Visa} />}
+ {isMasterCardNumber(cardNumbers) && <CardBrandImage src={Mastercard} alt="Mastercard" />}
+ {isVisaCardNumber(cardNumbers) && <CardBrandImage src={Visa} alt="Visa" />}📝 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.
| {isMasterCardNumber(cardNumbers) && <CardBrandImage src={Mastercard} />} | |
| {isVisaCardNumber(cardNumbers) && <CardBrandImage src={Visa} />} | |
| {isMasterCardNumber(cardNumbers) && <CardBrandImage src={Mastercard} alt="Mastercard" />} | |
| {isVisaCardNumber(cardNumbers) && <CardBrandImage src={Visa} alt="Visa" />} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Card/CardPreview.tsx` around lines 17 - 18, The CardPreview
rendering currently outputs CardBrandImage without alt text; update the JSX
where isMasterCardNumber(cardNumbers) and isVisaCardNumber(cardNumbers) are used
so each CardBrandImage includes a descriptive alt (e.g., "Mastercard logo" and
"Visa logo") and ensure the CardBrandImage component/prop types accept an alt
string (update its props/interface if necessary) so screen readers can announce
the brand.
| const CaptionStyle = styled.caption` | ||
| font-weight: 400; | ||
| font-size: 9.5px; | ||
| line-height: 100%; | ||
| letter-spacing: 0%; | ||
| color: #8b95a1; | ||
| `; |
There was a problem hiding this comment.
<caption>은 테이블 전용 요소입니다.
일반 section 안에서 쓰면 유효하지 않은 마크업이 됩니다. 이 위치라면 일반 텍스트 요소로 바꾸는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/CardInfoSection.tsx` around lines 35 - 41, The styled
component CaptionStyle currently uses the HTML <caption> element (CaptionStyle)
which is table-only; replace its tag with a semantically neutral inline/block
element (e.g., styled.span or styled.p) and update any usages of CaptionStyle in
CardInfoSection to use the new component so the markup becomes valid outside of
a table; ensure styling and accessibility remain the same (add an appropriate
ARIA role only if needed).
| export const getCardNumberErrorMessage = (values: string[]) => { | ||
| if (values.some((value) => value.length !== 4)) return ERROR_MESSAGE; | ||
| return null; |
There was a problem hiding this comment.
배열 길이도 함께 확인해 주세요.
현재 구현은 각 조각의 길이만 보므로, 4개가 아닌 배열이 들어와도 통과할 수 있습니다. 이 유틸이 설명하는 조건과 실제 판정 조건을 맞추는 편이 더 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/getCardNumberErrorMessage.ts` around lines 4 - 6,
getCardNumberErrorMessage currently only checks each chunk's length and can
accept arrays with wrong number of chunks; update the function to first assert
the incoming array length is exactly 4 and then validate that every element has
length 4, returning ERROR_MESSAGE if either check fails and null otherwise
(referencing getCardNumberErrorMessage and ERROR_MESSAGE to locate the logic).
| export const getCVCumberErrorMessage = (value: string) => { | ||
| if (value.length !== 3) return CVC_LENGTH_ERROR_MESSAGE; | ||
| return null; |
There was a problem hiding this comment.
길이만이 아니라 숫자 여부도 확인해야 합니다.
지금은 3글자면 '12a' 같은 값도 통과합니다. CVC 입력이 상위에서 숫자로만 정규화된다는 보장이 없다면, 이 validator만으로는 요구사항을 충분히 막지 못합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/getCVCNumberErrorMessage.ts` around lines 3 - 5, The validator
getCVCumberErrorMessage currently only checks length and will accept non-digit
strings like "12a"; update the function to require exactly three numeric digits
(e.g. validate with a regex for /^\d{3}$/ or by combining /^\d+$/ and length ===
3) and return CVC_LENGTH_ERROR_MESSAGE when that check fails; also fix the
function name to match the file intent (rename getCVCumberErrorMessage →
getCVCNumberErrorMessage) so callers and the file name are consistent.
| export const isMasterCardNumber = (cardNumbers: string[]) => { | ||
| if (cardNumbers[0].length < 2) return false; | ||
| if (cardNumbers[0][0] !== '5') return false; | ||
| const secondNumber = Number(cardNumbers[0][1]); | ||
| return 1 <= secondNumber && secondNumber <= 5; |
There was a problem hiding this comment.
빈 입력에 대한 방어가 필요합니다.
cardNumbers[0]를 먼저 접근해서 배열이 비어 있으면 첫 렌더에서 바로 예외가 납니다. 이 유틸을 초기 상태에도 쓰는지 확인하고, 빈 값 경로를 먼저 막아 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/isMasterCardNumber.ts` around lines 1 - 5, The function
isMasterCardNumber accesses cardNumbers[0] without guarding against an empty
array, causing an exception on initial render; update isMasterCardNumber to
first check that cardNumbers is an array with at least one element and that
cardNumbers[0] is a non-empty string (e.g., ensure cardNumbers?.length > 0 and
typeof cardNumbers[0] === 'string' and cardNumbers[0].length >= 2) and return
false early for empty/invalid input before performing the existing digit checks
(keep the existing checks using cardNumbers[0][0] and cardNumbers[0][1]).
| export const isMonthMatch = (value: string) => { | ||
| if (Number(value) > 12 || Number(value) < 1) return false; | ||
| if (2 !== value.length) return false; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
길이 판정 전에 숫자 문자열 여부를 함께 확인해 주세요.
지금은 Number(value)가 NaN이 되는 입력도 통과할 수 있어서, 길이만 2인 비숫자 값이 유효한 달로 판정될 수 있습니다. 월 검증 조건을 먼저 더 엄격하게 걸러보는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/isMonthMatch.ts` around lines 1 - 5, The isMonthMatch function
currently checks length and numeric range but allows non-numeric two-character
strings; update isMonthMatch to first ensure the input is a two-digit numeric
string (e.g., validate with a digit-only check or regex like /^\d{2}$/) before
converting to Number and checking range 01–12, and keep the explicit length
check only as part of that numeric-string validation so non-digit values (which
would produce NaN) are rejected.
There was a problem hiding this comment.
이런 상황이 발생할 수 있다는 것에 동의하고 있어요!
현재는 Number(value)가 NaN이 되는 값도 비교식에서 걸러지지 않아, 길이만 2인 비숫자 값이 유효한 월로 판정될 수 있어 보입니다.
보통 이런 검증을 엄격하게 가져가고 싶다면, false인 조건을 하나씩 제거하기보다 “true가 되기 위한 조건”을 명확히 if문으로 걸러서 찾는 방식이 더 안전하게 읽힐 것 같아요.
// 예시일 뿐이에요!
export const isMonthMatch = (value: string) => {
const isTwoDigitNumber = /^[0-9]{2}$/.test(value);
const month = Number(value);
const isValidMonthRange = month >= 1 && month <= 12;
if (isTwoDigitNumber && isValidMonthRange) return true;
return false;
};There was a problem hiding this comment.
조언 감사합니다!
현재 EXPInputWrapper에서 CardInfoInput를 호출할 때 isMonthMatch를 props로 전달하고 하고 CardInfoInput내부에서 해당 검증 이전에 handleInputChange 함수에서
if (Number.isNaN(Number(tmpValue))) {
onError('숫자만 입력할 수 있습니다.');
return;
}로 숫자가 아닌 입력에 대해서 검증을 처리하고 있습니다. 현재 방식은 isMonthMatch와 isLengthMatch가 호출 맥락에 의존하게 되고 만약 이들을 더 명시적이고 방어적인 코드로 수정한다면 조이가 말씀해주신대로 isMonthMatch와 isLengthMatch 모두 숫자 검증을 포함시킴으로써 함수가 호출 맥락에 의존하지 않게 될 수 있을 것 같네요!! true가 되기 위한 조건을 if문으로 명확히 걸러두는 방식도 참고해서 반영해보도록 하겠습니다
There was a problem hiding this comment.
isNaN을 체크하는 로직을 isNumeric라는 유틸 함수로 분리해서 재사용하는 로직으로 수정했습니다.
CardInfoInput 컴포넌트에 전달하던 validator에서 border처리를 맡았었는데 blur 시점에는 onChange가 호출되지 않아서 border가 업데이트 되지 않는 문제를 해결하기 위해 클릭 했는지 상태를 관리하는 hasTouched가 있는 Wrapper 컴포넌트에서 isError를 주입하는 방식으로 수정했습니다. 추가로 기존에 있던 validator는 error message를 위한 용도로 수정되었습니다!
| export const isVisaCardNumber = (cardNumbers: string[]) => { | ||
| if (cardNumbers[0].length < 1) { | ||
| return false; | ||
| } | ||
| if (cardNumbers[0][0] === '4') return true; | ||
| }; |
There was a problem hiding this comment.
빈 배열과 비-Visa 케이스를 함께 처리해 주세요.
cardNumbers[0]를 바로 읽어서 입력이 비어 있으면 예외가 납니다. 또 4로 시작하지 않는 경우 false가 아니라 undefined로 끝나서, 이 helper의 반환 계약이 깨집니다. 상위 상태가 항상 비어 있지 않은지 먼저 확인하고, 아니라면 이 helper가 스스로 방어하도록 정리해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/isVisaCardNumber.ts` around lines 1 - 6, The isVisaCardNumber
helper currently assumes cardNumbers[0] exists and returns undefined for
non-Visa values; update isVisaCardNumber to defensively handle empty arrays and
non-string entries by first checking that cardNumbers is an array with at least
one element and that cardNumbers[0] is a non-empty string, then return a boolean
for all code paths (true when the first character === '4', otherwise false).
Ensure you reference the function name isVisaCardNumber and adjust its early
returns so it never throws on empty input and always returns a boolean.
There was a problem hiding this comment.
소소하지만 return type이 boolean | undefined 여서 의도한게 아니라면 코드래빗의 추천 내용을 지키는 것도 좋아보입니다.
예시로 isVisaCardNumber(numbers) === false 로 사용하는 로직이 있다면 생각한 것과 다른 동작으로 진행되는 버그가 있을 수 있어보여요.
There was a problem hiding this comment.
앗 이 부분은 의도하지 않은 실수인 것 같아요..! 반영하겠습니다 감사합니다!!
coolchaem
left a comment
coolchaem
left a comment
There was a problem hiding this comment.
라바 안녕하세요!
리뷰를 맡은 조이입니다. 이번 1단계에서도 잘 부탁드립니다~
PR 설명을 정말 꼼꼼하게 남겨주셔서, 어떤 지점에서 어떤 트레이드오프를 고민하셨는지 잘 파악할 수 있었습니다. 특히 Input의 범용성과 카드 도메인 규칙 사이의 고민, CVC 상태를 어디에 둘지에 대한 고민, 에러 메시지 위치 문제를 해결하기 위해 wrapper 구조를 나눈 과정이 인상적이었습니다 👍
개인적으로는 CardInputWrapper가 공통 레이아웃과 에러 메시지 위치를 담당하고, CardNumberInputWrapper / EXPInputWrapper / CVCInputWrapper가 각 도메인 입력 구성과 에러 상태를 담당하는 현재 구조가 명시적이어서 가독성 측면에서 읽기 좋았습니다.
다만 몇 가지는 요구사항과 직접 연결된 버그가 있어보여 수정 요청으로 드립니다~
- 공백이 숫자로 유효하게 인식되는 문제
- 숫자가 아닌 값을 연속 입력했을 때 기존 숫자 일부가 사라지는 문제
그 외 논의하고 싶은 부분에 기재해주신 내용에 초점을 두어, 코드 위치에 코멘트를 달아두었습니다. 확인해보시고 궁금하신 것이 있다면 질문주세요! 👀
| export const isVisaCardNumber = (cardNumbers: string[]) => { | ||
| if (cardNumbers[0].length < 1) { | ||
| return false; | ||
| } | ||
| if (cardNumbers[0][0] === '4') return true; | ||
| }; |
There was a problem hiding this comment.
소소하지만 return type이 boolean | undefined 여서 의도한게 아니라면 코드래빗의 추천 내용을 지키는 것도 좋아보입니다.
예시로 isVisaCardNumber(numbers) === false 로 사용하는 로직이 있다면 생각한 것과 다른 동작으로 진행되는 버그가 있을 수 있어보여요.
| export const isMonthMatch = (value: string) => { | ||
| if (Number(value) > 12 || Number(value) < 1) return false; | ||
| if (2 !== value.length) return false; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
이런 상황이 발생할 수 있다는 것에 동의하고 있어요!
현재는 Number(value)가 NaN이 되는 값도 비교식에서 걸러지지 않아, 길이만 2인 비숫자 값이 유효한 월로 판정될 수 있어 보입니다.
보통 이런 검증을 엄격하게 가져가고 싶다면, false인 조건을 하나씩 제거하기보다 “true가 되기 위한 조건”을 명확히 if문으로 걸러서 찾는 방식이 더 안전하게 읽힐 것 같아요.
// 예시일 뿐이에요!
export const isMonthMatch = (value: string) => {
const isTwoDigitNumber = /^[0-9]{2}$/.test(value);
const month = Number(value);
const isValidMonthRange = month >= 1 && month <= 12;
if (isTwoDigitNumber && isValidMonthRange) return true;
return false;
};|
|
||
| export default function CardInfoSection({ title, caption, inputLabel, children }: CardSectionProps) { | ||
| return ( | ||
| <CardSectionStyle> |
There was a problem hiding this comment.
카드 정보 입력 영역에서 반복되는 title, caption, input label, children 구조를 CardInfoSection으로 잘 분리하신 것 같습니다. 덕분에 App.tsx에서 각 섹션의 의미가 잘 드러나서 읽기 좋았습니다 👍
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement, HTMLInputElement>) => { | ||
| const tmpValue = e.target.value; | ||
| if (Number.isNaN(Number(tmpValue))) { | ||
| onError('숫자만 입력할 수 있습니다.'); |
There was a problem hiding this comment.
저도 숫자만 입력할 수 있는 Input 형태의 컴포넌트 설계가 적합하다고 생각합니다!
오히려 사용자 관점에서 숫자만 입력되는 컴포넌트가 필요한게 명확하고, 재사용도 지금 상황에서 많이 되고 있으니까요~ 그런 의미에서 유효값 검증의 책임이 컴포넌트에 명확하게 있어도 된다고 느꼈어요.
문자열 입력까지 허용해야 하는 Input은 입력 규칙과 에러 피드백 방식이 달라질 가능성이 크기 때문에, 그런 요구가 생겼을 때 별도의 범용 Input으로 분리하는 편이 더 명확할 것 같습니다. 👍
| setEXPNumbers((prev) => prev.with(index, value)); | ||
| }; | ||
|
|
||
| const [cvc, setCVC] = useState(''); |
There was a problem hiding this comment.
CVC 상태를 왜 App까지 올렸는지 설명을 자세히 적어주셔서 이해하기 쉬웠습니다 👍
현재 1단계만 보면 CVC는 CardPreview에 표시되지 않기 때문에 CVCInputWrapper 내부에 둘 수도 있어 보입니다. 다만 다음 단계에서 CVC를 함께 검증하거나 카드 정보 전체를 제출해야 하는 흐름이 생길 수 있다는 점을 고려하면, 미리 App에서 함께 관리한 판단도 이해됩니다.
저도 다음 단계에서 곧 필요해질 상태라는 점을 알고 있었다면, CVC만 내부에 두기보다 카드번호/유효기간/CVC를 같은 레벨에서 관리하는 쪽이 더 일관적이라고 느꼈을 것 같아요.
다만 말씀하신 것처럼 “예측 가능한 변경에 미리 대응할지, 실제로 필요해질 때 끌어올릴지”는 트레이드오프가 있는 지점이라, 이번 판단의 근거를 PR 설명에 남겨주신 점이 좋았습니다 👏
| children: React.ReactNode; | ||
| } | ||
|
|
||
| export default function CardInputWrapper({ errorMessage, children }: CardInputWrapperProps) { |
There was a problem hiding this comment.
wrapper에 공통적인 스타일과 에러 메시지 표시 위치를 담당하는 공통 레이아웃 컴포넌트를 설계했다고 설명해주셨는데, 코드에서도 그렇게 읽혔습니다 👏
CardInputWrapper가 공통 레이아웃과 에러 메시지 위치를 담당하고, CardNumberInputWrapper / EXPInputWrapper / CVCInputWrapper가 각 섹션의 input 구성과 에러 상태를 담당하는 조합도 저는 가독성이 좋다고 느꼈어요.
제네릭으로 추상화하신 분들은 아마 재사용성에 더 초점을 두었을 것 같습니다. 다만 코드의 추구하는 방향에 따라 선택 지점은 달라질 수 있다고 생각해요.
라바는 이번 구조에서 영역별 에러 메시지와 유효성 검증 흐름이 더 잘 읽히는 쪽을 선택하신 것으로 보였습니다. 제네릭을 사용하면 중복은 줄일 수 있지만, 사용 방식에 따라 각 도메인의 차이가 잘 드러나지 않거나 오히려 가독성이 떨어질 수도 있을 것 같아요.
저였다면 영역별 커스터마이징될 소지가 충분히 많거나, 서비스가 아직 그렇게 복잡하지 않다고 판단되는 상황에서는 제네릭으로 강하게 추상화하기보다는 지금처럼 화면에 명시된 영역을 쉽게 읽을 수 있게 나누는 방식을 선택했을 것 같습니다 👍
| import styled from '@emotion/styled'; | ||
| import { useState } from 'react'; | ||
|
|
||
| type InputType = 'card-number' | 'exp' | 'cvc'; |
There was a problem hiding this comment.
width prop을 직접 전달하면 호출하는 모든 곳에서 카드번호/유효기간/CVC별 width 값을 알고 있어야 하니 관리하기 어렵다는 점에 공감합니다. 그런 의미에서 type을 두고 내부에서 width를 결정하려고 한 의도는 이해되었습니다 👍
아마 현재 구조에서는 CardNumberInputWrapper, EXPInputWrapper, CVCInputWrapper가 각 도메인의 검증과 입력 구성을 담당하고 있어서, CardInfoInput의 type은 width 정도만 담당하도록 남겨두신 것으로 이해했습니다. 혹시 다른 의도가 있었다면 알려주세요 👀
이 기준이라면, 저였다면 card-number, exp, cvc처럼 도메인 타입으로 두기보다는 small, medium, large 같은 width type이나 style type처럼 조금 더 추상화된 개념으로 정의하는 것도 고민해봤을 것 같습니다.
이런 variation은 Storybook에서 각 타입별 input을 보여주면, 동료가 “이 Input은 어떤 크기/상태로 사용할 수 있는지”를 확인하기에도 좋을 것 같아요.
There was a problem hiding this comment.
리뷰어분의 말씀대로 small, medium, large와 같은 width type으로 변경하는 것을 고려해 봤습니다.
width type으로 변경했을 때 호출부를 떠올려봤을 때 small, medium, large가 각각 어떤 디자인을 의미하는지, 어떤 Input을 보여주려고 하는 건지 알기 어렵다고 생각했습니다(기존에 type을 'card-number' | 'exp' | 'cvc'로 지정해둔 이유에 이 내용도 포함됩니다)
이러한 이유로
- type이 결정하는게 width밖에 없기 때문에 type으로 인자를 받는 건 적절하지 않다는 것
- 호출부에서 어떤 Input을 보여주는지 명확하게 알려줄 수 없었던 다른 해결법들
을 만족할 수 있는 방법은 초기에 고민했던 것처럼 width를 명시적으로 전달하는 방법 밖에 없다는 생각이 들었습니다. width를 직접 props로 부여하는 방식에 여전히 남아있던 '호출부에서 width가 몇인지 매번 알고 있어야한다'는 문제점은 제 코드에 적용하려고 보니 각 섹션의 Input Wrapper 컴포넌트(CardNumberInputWrapper, CVCInputWrapper, EXPInputWrapper)에서 해결되는 문제라고 생각했습니다. 결국 CardInfoInput을 직접 호출하는 곳은 없고 각 섹션 Input Wrapper 컴포넌트로 감싼 뒤에 Wrapper를 호출하는 형태기 때문에 우려했던 부분들도 문제가 되지 않을 수 있겠다 생각했습니다.
이러한 고민들의 최종 결과로 리뷰어분께서 언급해주신 width type을 적용해보기로 결정했습니다.
결정의 근거는 다음과 같습니다!
- width 자체를 props로 받는 방식은 도메인 특성이 적용된 CardInfoInput 컴포넌트에게는 너무 열려있는 선택지를 제공한다고 느껴졌습니다.
- 앞서 width type('small', 'medium', 'large')으로 props로 제공하는 방식이 각각 어떤 디자인을 의미하는지 알기 어렵다고 생각했지만 결국 CardInfoInput 컴포넌트는 Wrapper 컴포넌트로 감싸서 사용하기 때문에 어떤 섹션인지 컴포넌트 명에서 의미가 드러나고 크게 문제가 되지 않는다고 생각했습니다.
결과적으로 어떤 선택이 나은지 리뷰어분께 질문드리고 싶은데 어떻게 질문드려야할지 잘 모르겠어서 제 의사 결정 흐름을 남겨봤습니다..! 긴 글과 줏대없는 선택 죄송합니다..! ㅠㅠ
There was a problem hiding this comment.
의사 결정 흐름을 적어주셔서 어떤 고민을 하고 있는지 알았습니다!
현재 고민하고 계신 내용은 각 선택마다 장점/단점이 있어서 명확하게 이 선택을 해야한다의 정답은 없다고 생각하면 좋을 것 같네요. 지금 제가 드리는 답변은 라바가 어떤 설계를 하고 싶었고, 그것을 달성하기 위한 선택은 무엇일지를 리뷰한다는 입장으로 봐주시면 될 것 같습니다 👍
width를 외부에서 자유롭게 설정하는 것이 버그 혹은 지금 서비스 구현 취지와 맞지 않는다면 width type으로 선택하셔도 wrapper 안에서 사용하므로 큰 이슈가 없을 것 같다는 생각에 동의한바입니다~ 만약 추가적으로 확신이 서지 않는다면 width prop / width type / input type 이 CardInfoInput 컴포넌트가 재사용 혹은 확장성 혹은 범용성 등을 고려했을 때 장점/단점을 테이블로 한 번 정리해보면 어떨까요?
| onError('숫자만 입력할 수 있습니다.'); | ||
| return; | ||
| } | ||
| setValue(tmpValue); |
There was a problem hiding this comment.
숫자를 모두 입력한 상태에서 문자를 여러 번 누르면 입력된 카드 번호가 지워지는 버그가 있는 것 같습니다.
ex) 1234 입력 후 aa 연달아 누르기 or 12 입력 후 aa 연달아 누르기
그리고 공백도 아래 사진처럼 기입되고 있어서, 이 핸들러 로직을 한 번 점검해보면 좋겠습니다 🙄
현재는 Number(tmpValue)를 기준으로 숫자 여부를 판단하고 있는데, JavaScript의 숫자 변환 규칙 때문에 의도와 다른 값이 통과할 수 있어 보입니다.
Number(' ') // 0
There was a problem hiding this comment.
피드백 감사합니다! 말씀해주신 케이스에 대해서 원인을 찾아보고 해결한 후에 원인과 해결방법 남기도록 하겠습니다!!
There was a problem hiding this comment.
말씀해주신 Number(' ')과 같은 공백 입력이 검증되지 않는 버그는 수정했습니다! 하지만
숫자를 모두 입력한 상태에서 문자를 여러 번 누르면 입력된 카드 번호가 지워지는 버그
는 말씀해주신 대로 입력창에 입력해봤을 때 재현되지 않아서 어떤 버그인지 원인을 파악하지 못했습니다..! 코드상으로 숫자가 아닌 문자열을 입력했을 땐 아예 setValue가 되지 않도록 되어있어 말씀해주신 버그 상황 조금만 더 자세히 설명해주실 수 있으신지 궁금합니다..!
There was a problem hiding this comment.
재현이 되지 않으시다면 아마 제 os/브라우저에서만 발생하는 버그일 수도 있겠어요!
일단 제 환경은 윈도우11/chrome 사용하였습니다. 그리고 영문이 아니라 한글 입력 시 ime 조합하면서 발생하고 있어요!
입력: 1 입력 > ㅁ 입력 > ㅁ 입력
화면: 1 > 1 > (공백, 1사라짐)
만약 제 환경에서만 일어난다면 특정 기기 이슈로 생각하고 넘어가도 좋을 것 같습니다 👍
coolchaem
left a comment
coolchaem
left a comment
There was a problem hiding this comment.
라바 안녕하세요!
CardInfoInput width 관련 수정과 공백이 입력 가능한 오류 수정 반영해주신 것 확인하였습니다 👍 width type이 좋을지 width prop으로 넘기는게 좋을지 고민해주신 내용 잘 보았습니다.
어떤 부분에서 많은 고민이 있으셨는지 저도 공감이 갑니다! 저는 컴포넌트를 설계할 때 주요하게 생각하는 카테고리를 적어서 방법마다 장점/단점 테이블을 채워넣고 선택을 하는 편이에요. 그래서 확신이 더 필요하시다면, 해당 컴포넌트에서 어떤 설계를 목표했고 각 방법마다 장점/단점이 무엇일지 한 번 정리해보시는 것도 추천드려요~
지금 방식을 택한 라바의 선택이 합리적이라고 생각하고, 다음 단계를 진행하기에 적합한 상태로 보여서 approve로 변경해두겠습니다 👏 나머지는 다음 단계를 진행하면서 다듬어보시면 좋을 것 같습니다!
2 participants
🎯 페이먼츠
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
2단계
3단계
idle | loading | success | error네 가지로 명시적으로 관리한다.🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
Input의 범용성과 도메인 규칙이라는 트레이드 오프
type='text'로 두고 maxLength를 적용한 후 문자열 입력에 대해서는 javascript 문법으로 처리하는 방식과type="number"로 두고 문자열 입력은 input 태그에게 막도록 한 후에 maxLength를 javascript 문법으로 처리하는 방식중에서 고민했습니다. 결과적으로는 문자열 입력에 대해서도 에러 메세지를 띄워주기 위해서 type='text'로 고정해 두었는데 사실 만약 문자열 입력에 대해 에러 메세지를 띄워준다는 상황이 없었다면 어떻게 했어야 했을지 고민이 됐습니다. 컴포넌트에 범용성이라는 기준에서의 고민이었는데요, 문자열 입력을 내부에서 차단하면 숫자 외의 입력이 필요한 상황에서는 재사용할 수 없는 상황이 발생하게 되는데 사실CardInfoInput라는 이름처럼 Card 정보를 입력하는 Input에서는 문자열 입력이 불가능한게 도메인 규칙에 오히려 더 맞을 수 있다는 합리화가 되기도 했습니다. 이러한 고민의 결과로 범용 적인 Input 컴포넌트가 필요하다면 별도로 설계하면 되고, 도메인 규칙을 컴포넌트 수준에서 강제하는 것은 오히려 책임이 명확한 설계라고 판단했습니다.CVC 상태를 App.tsx에 둔 이유
CVCInputWrapper내부에 두는 것도 가능했습니다. 하지만 추후에 최종 유효성 검사, CardPreview에 CVC 렌더링과 같이 예측 가능한 요구사항 변경이 생기는 것을 고려하여(사실 step2 피그마 구현을 미리 봤을 때 CVC 상태를 전역으로 관리해야할 필요가 있다고 판단했습니다..!) 전역으로 관리하는 방식이 더 유리하다고 생각했습니다. 또한 각 InputWrapper 컴포넌트가 value, setValue, validator를 외부에서 주입받는 일관된 인터페이스를 갖도록 설계했는데 CVC도 이 구조를 따른다면 어떤 Wrapper든 동일한 방식으로 사용할 수 있어서 일관된 인터페이스로 인해 컴포넌트를 사용하는 사람도 더욱 편리할 것이라고 생각했습니다. 다만 지금 시점에서는 CVC 상태를 외부에서 사용하는 곳이 없기 때문에, 실제로 필요해지는 시점에 끌어올리는 방식이 더 적절했을 수도 있습니다. 이러한 구조 고민의 결과로 예측 가능한 변경에 미리 대응하는 것과 필요해질 때 대응하는 것은 트레이드오프 관계이고, 이 판단은 상황에 따라 달라진다는 점을 알 수 있었습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
각 Input의 값에 따라 에러 메세지를 실시간으로 보여주고 싶었는데, 이를 Input 컴포넌트 내부에서 관리하면 여러 Input이 묶인 그룹 전체(여러 Input을 Wrapping하는 컴포넌트)에 에러 메세지를 동일한 위치에 띄우기 어려웠습니다. 이 문제를 해결하기 위해
CardInputWrapper라는 에러 메세지 표시 위치와 공통 레이아웃 담당 컴포넌트,CardNumberInputWrapper / EXPInputWrapper / CVCInputWrapper라는 각 섹션의 Input 구성과 에러 상태를 관리하는 컴포넌트를 구현했습니다. 에러 상태를 각 섹션 Wrapper에 두고 CardInfoInput에 onError 콜백을 전달해 Input이 유효하지 않은 값을 감지했을 때 상위로 알리는 방식으로 구현했습니다. 하지만 다른 크루들의 코드를 보니 제네릭 타입을 활용해 하나의 컴포넌트에서 세 섹션을 모두 처리하도록 추상화한 경우도 있었습니다.(저도 처음에는 하나의 레이아웃 컴포넌트로 모든 섹션을 처리하려고 했지만 현재 방식이 각 섹션별 에러 상태 관리를 더 용이하게 할 수 있고 섹션별 유효성 검사 규칙이 다름, Input 구성이 다름 등의 이유로 변경했습니다.) 현재 상황에서 추상화를 통해 얻는 재사용성과 각 도메인의 차이를 명시적으로 드러내는 것 사이에서 어떤 방식이 더 적절하고 그 판단의 기준은 어떤 것으로 가져가는게 적절한지 궁금합니다. 또한 현재 구조에서 역할 분리의 방향(UI용 분리, 섹션별 에러 상태 관리용 분리)이 적절한지, 리뷰어 분이라면 이런 상황에 어떤 방식으로 구현하셨을 지 의견을 듣고 싶습니다.현재 CardInfoInput 컴포넌트를 type prop('card-number' / 'exp' / 'cvc')에 따라 내부적으로 width를 결정하도록 설계했습니다. AI에게 코드 리뷰를 요청했을 때 type에 따라서 결정되는게 width밖에 없기 때문에 type으로 width를 조절하는 방식 보다는 width props를 전달하는 방식이 더 적절하다는 의견을 받았습니다. 하지만 저는 type prop 방식이 더 선언적이라고 판단했는데 그 이유는
type에 따른 규칙이 어떻게 적용되는지 알 수 없기 때문에 호출부에서 어떤 Input이 렌더링되는지 이해하기 어려울 수 있다고 생각했습니다.
이러한 내용들을 근거로 현재 방식대로 구현했는데 이 판단이 적절한지, 아니면 현재처럼 width 하나만 결정되는 상황에서는 단순하게 width prop으로 처리하는 것이 맞는지 리뷰어분의 의견을 듣고 싶습니다!
✅ 리뷰어 체크 포인트
1. Form 상태 관리 & Custom Hook 분리
2. 입력 UI 흐름과 UX
3. 컴포넌트 구조 및 재사용성
4. 상태 기반 유효성 검사 및 확인 버튼 활성화
5. 비동기 상태 · 네트워크 경계 · 통합 테스트
idle | loading | success | error네 가지로 명시적으로 관리하고,isLoading/error를 별도 boolean으로 쪼개지 않았는가?POST/GET/DELETE /cards와 400 시나리오까지 포함하여 네트워크 경계에서 동작하는가?fetch·axios를 모킹하지 않고, MSW + RTL로 사용자 관점에서 작성되었는가?getByRole → getByText → getByLabelText → getByTestId우선순위를 따르고, 비동기 요소에findBy*를 사용했는가?