[페이먼츠 2단계 - hooks & state] 포도(조은향) 미션 제출합니다.#543
Conversation
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: less <lejin0906@gmail.com>
Co-authored-by: goodsmell <ck0302@naver.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: goodsmell <ck0302@naver.com>
Co-authored-by: goodsmell <ck0302@naver.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: lee-eojin <lejin0906@gmail.com>
Co-authored-by: goodsmell <ck0302@naver.com>
Co-authored-by: goodsmell <ck0302@naver.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:
워크스루이 PR은 Vite, React 19, TypeScript, Emotion을 사용한 신용카드 등록 폼 애플리케이션을 완전히 구현합니다. 타입 및 검증 인프라에서 시작하여 폼 상태 관리 훅, 기초 UI 컴포넌트, 폼 레이아웃 래퍼, 카드 입력 섹션 (카드 번호, 유효기간, CVC, 비밀번호, 카드사), 페이지 컴포넌트 및 라우팅, Storybook 설정 및 상호작용 테스트를 포함합니다. 예상 코드 리뷰 노력🎯 4 (복잡함) | ⏱️ ~45분 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 12
🧹 Nitpick comments (8)
my-app/src/types.ts (1)
1-6: ⚡ Quick win
CardInfo의 배열 타입이 너무 느슨합니다.
numbers/expiry가string[]라서 길이 제약(예: 4개 블록, MM/YY 2개 블록)이 타입에서 보장되지 않습니다. 튜플 타입으로 좁히면 카드 포맷 관련 버그를 컴파일 단계에서 더 빨리 잡을 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/types.ts` around lines 1 - 6, The CardInfo type uses loose arrays for numbers and expiry—change CardInfo to use tuple types to enforce expected block counts: replace numbers: string[] with numbers: [string, string, string, string] and expiry: string[] with expiry: [string, string]; update any code constructing or deconstructing CardInfo (e.g., places that build or spread numbers/expiry) to match the tuple shape and adjust function signatures or destructuring that assume variable-length arrays.my-app/src/components/CardForm/ExpiryDateInputSection/ExpiryDateInputSection.tsx (2)
19-25: ⚡ Quick win자동 포커스 이동 패턴을 고려해보셨나요?
CardNumberInputSection에서는 한 칸이 채워지면 자동으로 다음 입력 필드로 포커스가 이동합니다. 유효기간 입력(MM/YY)에서도 동일한 UX 패턴을 적용하면 사용자 경험이 더 좋아질 것 같은데 어떻게 생각하시나요?힌트:
useRef를 활용해 다음 입력 필드의 DOM 참조를 저장하고,maxLength에 도달했을 때.focus()를 호출하는 방식을 생각해보세요.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/components/CardForm/ExpiryDateInputSection/ExpiryDateInputSection.tsx` around lines 19 - 25, The expiry inputs currently update values in handleChange but don't auto-move focus; modify ExpiryDateInputSection to store refs for each input (useRef<HTMLInputElement[]>([]) or separate refs), assign them to the MM and YY inputs, and in handleChange after setting newValues and before calling onChange, check if value.replace(/\D/g,"").length >= maxLength (or === maxLength) and call refs[index + 1]?.focus() to move to the next field; ensure bounds-checking so you don't focus past the last input, and keep existing clearError() and onChange(newValues) behavior intact.
34-50: ⚡ Quick win접근성 속성 추가를 고려해주세요.
입력 필드에 검증 오류가 있을 때 스크린 리더 사용자를 위한
aria-invalid나aria-describedby속성을 추가하면 어떨까요?현재는 시각적으로만 오류를 표시하고 있는데(빨간 테두리), 접근성 측면에서 개선할 여지가 있어 보입니다. WCAG 가이드라인에서 폼 검증 관련 접근성 권장사항을 참고해보시면 도움이 될 것 같습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/components/CardForm/ExpiryDateInputSection/ExpiryDateInputSection.tsx` around lines 34 - 50, The inputs rendered in the inputValues.map currently only show errors visually; update the <input> elements (in the ExpiryDateInputSection component where inputValues.map, handleChange, handleBlur, baseInputStyle, placeholders and errorIndex are used) to include accessibility attributes: set aria-invalid={errorIndex === i} and when an error message exists add aria-describedby pointing to a unique id for that field's error (e.g., `${fieldName}-error-${i}`), and ensure the error element renders with that id so screen readers can announce the validation message.my-app/src/hooks/useCardForm.ts (2)
48-53: ⚖️ Poor tradeoffCVC 길이 가정에 대해 생각해보세요.
현재 코드는 CVC가 항상 3자리라고 가정하고 있습니다.
질문: American Express 카드는 4자리 CVC를 사용하는데, 이런 경우를 지원해야 할까요?
brand정보를 활용해서 브랜드별로 다른 CVC 길이를 검증하는 방법을 고려해보셨나요?힌트: 16-17번 줄에서 이미
brand를 감지하고 있고,getFieldConfig를 통해 브랜드별 설정을 가져오고 있습니다. CVC 길이도 이런 방식으로 관리할 수 있을까요?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/hooks/useCardForm.ts` around lines 48 - 53, The cvcHandler currently assumes a 3-digit CVC; update it to use the card brand-specific CVC length instead: read the current brand (from the same hook where `brand` is tracked) and call `getFieldConfig(brand)` to obtain the expected `cvcLength` (fall back to 3 if missing), then setCardInfo as before and only call setStep((prev) => Math.max(prev, 4)) when cvc.length >= cvcLength; reference `cvcHandler`, `setCardInfo`, `setStep`, `brand`, and `getFieldConfig` when making this change.
55-57: 💤 Low value비밀번호 핸들러가 단계를 진행시키지 않는 이유가 있나요?
다른 핸들러들(
cardNumberHandler,expiryHandler,cvcHandler)은 모두 입력이 완료되면 다음 단계로 진행시키는데,passwordHandler만 단계를 진행시키지 않습니다.질문:
- 비밀번호가 마지막 입력 필드여서 단계를 진행시킬 필요가 없는 건가요?
- 아니면 비밀번호 입력 완료 시점에 특별한 동작(예: 제출 버튼 활성화)이 필요한가요?
- 69번 줄의
isPasswordValid는 길이가 2인지 확인하는데, 왜 2자리인지 설명하는 주석이 있으면 좋을 것 같습니다. 한국 카드의 특성인가요?코드의 의도를 명확히 하기 위해 주석을 추가하거나, 패턴을 일관되게 유지하는 것을 고려해보세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/hooks/useCardForm.ts` around lines 55 - 57, The passwordHandler currently only updates state and doesn't advance the form like cardNumberHandler/expiryHandler/cvcHandler; either make its behavior consistent by advancing the step when the password meets validation (e.g., inside passwordHandler call setStep(prev => prev + 1) when isPasswordValid(password) is true) or explicitly document why it should not advance (e.g., final field that instead enables the submit action). Also add a clarifying comment at the isPasswordValid check explaining why length === 2 is used (e.g., Korea-specific card PIN2 requirement) so the intent is clear.my-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.stories.tsx (2)
36-44: ⚡ Quick win쿼리 방식의 일관성을 확인해주세요.
ErrorNonNumeric스토리(38번 줄)에서는within(canvasElement)를 사용하는데,FilledValid스토리(30번 줄)에서는querySelector를 직접 사용하고 있습니다.두 스토리에서 동일한 쿼리 패턴을 사용하는 것이 코드 일관성 측면에서 더 좋지 않을까요?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.stories.tsx` around lines 36 - 44, The two stories use inconsistent DOM querying: ErrorNonNumeric's play uses within(canvasElement) while FilledValid uses canvasElement.querySelector directly; make them consistent by standardizing on one approach (prefer using within for RTL consistency). Update the FilledValid story's play function to obtain the input via within(canvasElement).getByRole or getByLabelText (or update ErrorNonNumeric to match if you prefer querySelector) so both stories use the same query pattern (refer to the play functions of ErrorNonNumeric and FilledValid, canvasElement, within, querySelector).
30-30: ⚡ Quick win접근성을 고려한 쿼리 방식을 사용해보세요.
querySelector로 직접 DOM을 선택하는 대신 Testing Library의 역할 기반 쿼리(getByRole)를 사용하면 어떨까요? 이렇게 하면 실제 사용자 관점에서 테스트할 수 있고, 접근성 문제도 함께 검증할 수 있습니다.힌트: 비밀번호 입력 필드가 어떤 ARIA role을 가져야 할지 생각해보세요. 다른 스토리 파일들에서는
textboxrole을 사용하고 있습니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.stories.tsx` at line 30, Replace the DOM query that sets const input = canvasElement.querySelector('input[type="password"]')! with a role-based Testing Library query: use the within helper on canvasElement and call getByRole('textbox', { name: /password/i }) (or the exact accessible label used in this story) to locate the password field; update the variable name (input) to use that result and add/import within from the testing-library/testing helpers used elsewhere in the repo (e.g., `@storybook/testing-library` or `@testing-library/dom`) so the play function uses an accessibility-focused query.my-app/src/components/CardForm/CardFormLayout/CardFormLayout.tsx (1)
32-45: ⚡ Quick win유효하지 않은 상태에서도 제출 버튼은 노출하고
disabled로 처리해 주세요.버튼이 조건부로 사라지면 액션 위치가 바뀌고 현재 진행 상태를 파악하기 어렵습니다. 버튼을 항상 렌더링한 뒤
disabled와 함께 상태 안내를 주는 쪽이 UX/접근성 측면에서 더 안정적입니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@my-app/src/components/CardForm/CardFormLayout/CardFormLayout.tsx` around lines 32 - 45, The submit button in CardFormLayout currently renders only when isValid; always render the button instead and control interactivity via the disabled prop (e.g., disabled={!isValid}) so the action location remains stable; update the JSX in CardFormLayout to remove the conditional around the <button> and add disabled based on isValid, plus an accessible hint (aria-disabled or aria-describedby) tied to the form validation state to convey why it’s disabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@my-app/index.html`:
- Around line 2-8: The document's lang attribute is set to "en" while the UI
content is Korean; update the HTML root element's lang attribute (the <html
lang="en"> declaration) to "ko" so assistive technologies and screen readers use
Korean pronunciation/interpretation; ensure any language-specific metadata
(e.g., title or meta tags) aligns with the change.
In `@my-app/src/components/Card/Card.tsx`:
- Around line 63-71: The current conditional renders the <img> whenever `brand`
is truthy which can result in a broken image if `LOGO_MAP[brand]` is undefined;
change the guard to ensure the mapped logo exists (e.g., compute `const logo =
LOGO_MAP[brand]` and only render the <img> when `logo` is defined) so `src`
never receives undefined and only valid logos are rendered; update references in
the Card component (the `brand` prop, `LOGO_MAP`, and the <img> element)
accordingly.
In
`@my-app/src/components/CardForm/CardNumberInputSection/CardNumberInputSection.tsx`:
- Around line 22-33: The auto-focus check in handleChange uses the raw input
length (value.length) which can be larger than the sanitized numeric string;
update the condition to use the sanitized value stored in newValues (e.g.,
newValues[index].length) so the focus moves only when the cleaned digits reach
fieldConfig[index]; keep the rest of the logic (clearError, onChange,
inputRefs.current[index + 1]?.focus()) the same and ensure you reference
handleChange, inputValues, newValues, fieldConfig, inputRefs, clearError and
onChange when making the change.
In `@my-app/src/components/CardForm/CvcInputSection/CvcInputSection.tsx`:
- Around line 16-30: The CVC input never shows validation because
CvcInputSection passes a hardcoded errorMessage="" to InputSectionLayout and
lacks blur-time validation; update CvcInputSection to (1) derive an error string
from the existing validation state/validator (e.g., validateCvc or cvcError
state) and pass that value into the InputSectionLayout errorMessage prop instead
of "" and (2) wire an onBlur handler on the <input> that triggers the same
validation used in handleChange (e.g., call handleBlur or validateCvc) so errors
are computed and displayed on blur as well as change; reference
InputSectionLayout, errorMessage, CvcInputSection, handleChange, and add an
onBlur that updates the validation state.
In `@my-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.tsx`:
- Around line 25-27: The component's props currently reference React.ReactNode
which fails under the project's TS config; update the top of
InputSectionLayout.tsx to add an explicit type import—import type { ReactNode }
from "react"—and change the props signature to use ReactNode instead of
React.ReactNode (the symbol to edit is the props object in the
InputSectionLayout component where children?: React.ReactNode is declared).
In
`@my-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.tsx`:
- Around line 15-18: The onChange handler (onChange) currently strips non-digits
before calling onValueHandler which prevents the blur validation path and so
non-numeric error messages never surface; instead preserve the raw input in
onChange (and still call clearError) and only perform numeric
normalization/validation in the blur handler (the existing blur validator) where
you should compute sanitized = value.replace(/\D/g, "") and call
onValueHandler(sanitized) or set an error if validation fails; apply the same
change to the other occurrence at the second onChange (line ~34) so validation
and transformation are separated.
In `@my-app/src/components/Layout/Layout.tsx`:
- Around line 17-23: 현재 Layout 컴포넌트의 스타일 블록에서 사용된 "height: 700px" 설정은 작은 뷰포트에서
하단 콘텐츠가 잘리는 원인이므로, 해당 CSS(Layout 컴포넌트의 컨테이너 스타일 블록, 현재에 'height: 700px'로 지정된 곳)를
변경해 고정 높이를 제거하고 반응형으로 처리하세요: "height: 700px"을 삭제하고 대신 "min-height: 700px;
max-height: 100vh; overflow-y: auto;"를 적용해 필요 시 스크롤이 가능하도록 만드세요.
In `@my-app/src/components/Select/Select.tsx`:
- Around line 52-87: The option list rendered by options.map inside the Select
component (the <ul> with <li> that calls handleSelect) is not
keyboard-accessible; change each option from a plain clickable <li> to an
accessible, focusable control (either a <button> or an element with
role="option" and tabindex={0}) and implement keyboard handlers: add an internal
activeIndex state and onKeyDown on the list (or each option) to support
ArrowUp/ArrowDown to move focus/activeIndex and Enter/Space to call
handleSelect(option); also expose proper ARIA attributes (aria-selected or
aria-activedescendant on the list and role="listbox"/role="option") and keep the
existing visual highlight logic using value or activeIndex so keyboard users can
navigate and select options.
In `@my-app/src/constants/cardBrand.ts`:
- Line 15: The MasterCard detection only checks /^5[1-5]/ so it misses the
2221–2720 BIN range; update detectBrand to also detect 2-series MasterCards by
checking the first 4 digits numerically (e.g., parseInt(number.slice(0,4),10)
between 2221 and 2720) or replace the regex with one that covers both ranges,
and return "master" when either condition matches (update the condition
currently using /^5[1-5]/ in the detectBrand implementation).
In `@my-app/src/hooks/useCardForm.ts`:
- Around line 36-39: companyHandler currently advances the step to 2
unconditionally; change it to validate the input (use the existing
isCompanyValid check or ensure company !== ''/meets validation rules) before
calling setStep, mirroring the pattern used in cardNumberHandler and
expiryHandler: first update state via setCardInfo, then if the value passes
validation call setStep(prev => Math.max(prev, 2)), otherwise do not advance
(and optionally keep existing error state handling).
In `@my-app/src/pages/CardComplete.tsx`:
- Around line 7-10: The CardComplete component currently assumes route state
exists and directly reads numbers[0] and brand, which breaks when a user lands
on /complete via refresh or direct link; update the component to validate the
state from useLocation() (symbols: useLocation, useNavigate, state, numbers,
brand) and if numbers is empty or brand is falsy, perform a safe redirect using
navigate('/') or render a fallback UI (e.g., a loading/empty state) instead of
rendering numbers[0]/brand; ensure this validation runs early (on mount or
before JSX render) so references to numbers[0] and brand are never used when
undefined.
In `@my-app/src/pages/CardForm.tsx`:
- Around line 27-31: The handleSubmit currently navigates to "/complete" passing
the full cardInfo (including PAN/CVC/password) in route state; change
handleSubmit to remove sensitive fields before navigation by either (a)
generating a masked object (e.g., maskedCardInfo containing brand and last4) or
(b) sending cardInfo to a tokenization function (e.g., tokenizeCard or
submitCardToServer) and passing only the server token or masked result; update
the call to navigate("/complete", { state: { /* maskedCardInfo or token */ ,
brand } }) and ensure the original cardInfo is cleared/discarded after
tokenization to avoid keeping plaintext secrets in memory/state.
---
Nitpick comments:
In `@my-app/src/components/CardForm/CardFormLayout/CardFormLayout.tsx`:
- Around line 32-45: The submit button in CardFormLayout currently renders only
when isValid; always render the button instead and control interactivity via the
disabled prop (e.g., disabled={!isValid}) so the action location remains stable;
update the JSX in CardFormLayout to remove the conditional around the <button>
and add disabled based on isValid, plus an accessible hint (aria-disabled or
aria-describedby) tied to the form validation state to convey why it’s disabled.
In
`@my-app/src/components/CardForm/ExpiryDateInputSection/ExpiryDateInputSection.tsx`:
- Around line 19-25: The expiry inputs currently update values in handleChange
but don't auto-move focus; modify ExpiryDateInputSection to store refs for each
input (useRef<HTMLInputElement[]>([]) or separate refs), assign them to the MM
and YY inputs, and in handleChange after setting newValues and before calling
onChange, check if value.replace(/\D/g,"").length >= maxLength (or ===
maxLength) and call refs[index + 1]?.focus() to move to the next field; ensure
bounds-checking so you don't focus past the last input, and keep existing
clearError() and onChange(newValues) behavior intact.
- Around line 34-50: The inputs rendered in the inputValues.map currently only
show errors visually; update the <input> elements (in the ExpiryDateInputSection
component where inputValues.map, handleChange, handleBlur, baseInputStyle,
placeholders and errorIndex are used) to include accessibility attributes: set
aria-invalid={errorIndex === i} and when an error message exists add
aria-describedby pointing to a unique id for that field's error (e.g.,
`${fieldName}-error-${i}`), and ensure the error element renders with that id so
screen readers can announce the validation message.
In
`@my-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.stories.tsx`:
- Around line 36-44: The two stories use inconsistent DOM querying:
ErrorNonNumeric's play uses within(canvasElement) while FilledValid uses
canvasElement.querySelector directly; make them consistent by standardizing on
one approach (prefer using within for RTL consistency). Update the FilledValid
story's play function to obtain the input via within(canvasElement).getByRole or
getByLabelText (or update ErrorNonNumeric to match if you prefer querySelector)
so both stories use the same query pattern (refer to the play functions of
ErrorNonNumeric and FilledValid, canvasElement, within, querySelector).
- Line 30: Replace the DOM query that sets const input =
canvasElement.querySelector('input[type="password"]')! with a role-based Testing
Library query: use the within helper on canvasElement and call
getByRole('textbox', { name: /password/i }) (or the exact accessible label used
in this story) to locate the password field; update the variable name (input) to
use that result and add/import within from the testing-library/testing helpers
used elsewhere in the repo (e.g., `@storybook/testing-library` or
`@testing-library/dom`) so the play function uses an accessibility-focused query.
In `@my-app/src/hooks/useCardForm.ts`:
- Around line 48-53: The cvcHandler currently assumes a 3-digit CVC; update it
to use the card brand-specific CVC length instead: read the current brand (from
the same hook where `brand` is tracked) and call `getFieldConfig(brand)` to
obtain the expected `cvcLength` (fall back to 3 if missing), then setCardInfo as
before and only call setStep((prev) => Math.max(prev, 4)) when cvc.length >=
cvcLength; reference `cvcHandler`, `setCardInfo`, `setStep`, `brand`, and
`getFieldConfig` when making this change.
- Around line 55-57: The passwordHandler currently only updates state and
doesn't advance the form like cardNumberHandler/expiryHandler/cvcHandler; either
make its behavior consistent by advancing the step when the password meets
validation (e.g., inside passwordHandler call setStep(prev => prev + 1) when
isPasswordValid(password) is true) or explicitly document why it should not
advance (e.g., final field that instead enables the submit action). Also add a
clarifying comment at the isPasswordValid check explaining why length === 2 is
used (e.g., Korea-specific card PIN2 requirement) so the intent is clear.
In `@my-app/src/types.ts`:
- Around line 1-6: The CardInfo type uses loose arrays for numbers and
expiry—change CardInfo to use tuple types to enforce expected block counts:
replace numbers: string[] with numbers: [string, string, string, string] and
expiry: string[] with expiry: [string, string]; update any code constructing or
deconstructing CardInfo (e.g., places that build or spread numbers/expiry) to
match the tuple shape and adjust function signatures or destructuring that
assume variable-length arrays.
🪄 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: a8c228ea-ef1c-4579-922e-1a13f131b3dd
⛔ Files ignored due to path filters (11)
my-app/package-lock.jsonis excluded by!**/package-lock.jsonmy-app/public/favicon.svgis excluded by!**/*.svgmy-app/public/icons.svgis excluded by!**/*.svgmy-app/src/assets/amexLogo.pngis excluded by!**/*.pngmy-app/src/assets/completeCheck.pngis excluded by!**/*.pngmy-app/src/assets/dinersLogo.pngis excluded by!**/*.pngmy-app/src/assets/ic_chevron_down.svgis excluded by!**/*.svgmy-app/src/assets/ic_chevron_up.svgis excluded by!**/*.svgmy-app/src/assets/masterLogo.pngis excluded by!**/*.pngmy-app/src/assets/unionpayLogo.pngis excluded by!**/*.pngmy-app/src/assets/visaLogo.pngis excluded by!**/*.png
📒 Files selected for processing (43)
my-app/.gitignoremy-app/.storybook/main.tsmy-app/.storybook/preview.tsmy-app/README.mdmy-app/eslint.config.jsmy-app/index.htmlmy-app/package.jsonmy-app/src/App.tsxmy-app/src/components/Button/Button.stories.tsxmy-app/src/components/Button/Button.tsxmy-app/src/components/Card/Card.stories.tsmy-app/src/components/Card/Card.tsxmy-app/src/components/CardForm/CardCompanySelectSection/CardCompanySelectSection.stories.tsxmy-app/src/components/CardForm/CardCompanySelectSection/CardCompanySelectSection.tsxmy-app/src/components/CardForm/CardFormLayout/CardFormLayout.tsxmy-app/src/components/CardForm/CardNumberInputSection/CardNumberInputSection.stories.tsxmy-app/src/components/CardForm/CardNumberInputSection/CardNumberInputSection.tsxmy-app/src/components/CardForm/CvcInputSection/CvcInputSection.stories.tsxmy-app/src/components/CardForm/CvcInputSection/CvcInputSection.tsxmy-app/src/components/CardForm/ExpiryDateInputSection/ExpiryDateInputSection.stories.tsxmy-app/src/components/CardForm/ExpiryDateInputSection/ExpiryDateInputSection.tsxmy-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.stories.tsxmy-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.tsxmy-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.stories.tsxmy-app/src/components/CardForm/PasswordInputSection/PasswordInputSection.tsxmy-app/src/components/Layout/Layout.tsxmy-app/src/components/Select/Select.stories.tsxmy-app/src/components/Select/Select.tsxmy-app/src/constants/cardBrand.tsmy-app/src/constants/cardCompanies.tsmy-app/src/hooks/useCardForm.tsmy-app/src/hooks/useInputValidation.tsmy-app/src/index.cssmy-app/src/main.tsxmy-app/src/pages/CardComplete.tsxmy-app/src/pages/CardForm.tsxmy-app/src/types.tsmy-app/src/utils/validators.tsmy-app/tsconfig.app.jsonmy-app/tsconfig.jsonmy-app/tsconfig.node.jsonmy-app/vite.config.tsmy-app/vitest.shims.d.ts
| <html lang="en"> | ||
| <head> | ||
| <meta charset="UTF-8" /> | ||
| <link rel="icon" type="image/svg+xml" href="/favicon.svg" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>my-app</title> | ||
| <link href="https://fonts.googleapis.com/css2?family=Noto+Sans+KR:wght@400;500;700&display=swap" rel="stylesheet"> |
There was a problem hiding this comment.
문서 언어 설정을 실제 UI 언어와 맞춰 주세요.
현재 lang="en"인데 화면 텍스트가 한국어 중심이라 보조기기 발음/해석 정확도가 떨어질 수 있습니다. lang="ko"로 맞추는 쪽이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/index.html` around lines 2 - 8, The document's lang attribute is set
to "en" while the UI content is Korean; update the HTML root element's lang
attribute (the <html lang="en"> declaration) to "ko" so assistive technologies
and screen readers use Korean pronunciation/interpretation; ensure any
language-specific metadata (e.g., title or meta tags) aligns with the change.
| {brand && ( | ||
| <img | ||
| alt="카드 브랜드 이미지" | ||
| css={css` | ||
| width: 36px; | ||
| height: 22px; | ||
| `} | ||
| src={LOGO_MAP[brand]} | ||
| /> |
There was a problem hiding this comment.
알 수 없는 brand 값에서 깨진 로고 이미지가 렌더링될 수 있습니다.
brand가 truthy이기만 하면 렌더링되므로, LOGO_MAP에 없는 값이면 src가 undefined가 됩니다. LOGO_MAP[brand] 존재 여부까지 확인해 렌더링을 가드하는 편이 안전합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/components/Card/Card.tsx` around lines 63 - 71, The current
conditional renders the <img> whenever `brand` is truthy which can result in a
broken image if `LOGO_MAP[brand]` is undefined; change the guard to ensure the
mapped logo exists (e.g., compute `const logo = LOGO_MAP[brand]` and only render
the <img> when `logo` is defined) so `src` never receives undefined and only
valid logos are rendered; update references in the Card component (the `brand`
prop, `LOGO_MAP`, and the <img> element) accordingly.
| const handleChange = (index: number, value: string) => { | ||
| const newValues = [...inputValues]; | ||
| newValues[index] = value.replace(/\D/g, ""); | ||
|
|
||
| clearError(); | ||
| onChange(newValues); | ||
|
|
||
| // 현재 칸이 꽉 찼고, 마지막 칸이 아닐 때 다음칸 Input Dom에 포커스 이동. | ||
| if (value.length === fieldConfig[index] && index < fieldConfig.length - 1) { | ||
| inputRefs.current[index + 1]?.focus(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
자동 포커스 이동 조건을 다시 확인해주세요.
30번 줄의 포커스 이동 조건이 의도와 다르게 동작할 수 있습니다.
현재 로직:
newValues[index] = value.replace(/\D/g, ""); // 숫자만 추출
// ...
if (value.length === fieldConfig[index] && ...) { // 원본 value 길이 체크질문: 사용자가 "abc1234"를 입력하면 어떻게 될까요?
value.length는 7이지만newValues[index]는 "1234"입니다
포커스 이동 조건을 어떤 값을 기준으로 판단해야 할까요? 원본 입력 길이일까요, 아니면 정제된 값의 길이일까요?
힌트: 24번 줄에서 정제한 값을 사용해서 길이를 확인하면 더 정확할 것 같습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@my-app/src/components/CardForm/CardNumberInputSection/CardNumberInputSection.tsx`
around lines 22 - 33, The auto-focus check in handleChange uses the raw input
length (value.length) which can be larger than the sanitized numeric string;
update the condition to use the sanitized value stored in newValues (e.g.,
newValues[index].length) so the focus moves only when the cleaned digits reach
fieldConfig[index]; keep the rest of the logic (clearError, onChange,
inputRefs.current[index + 1]?.focus()) the same and ensure you reference
handleChange, inputValues, newValues, fieldConfig, inputRefs, clearError and
onChange when making the change.
| <InputSectionLayout title="CVC번호를 입력해 주세요" message="" tag="CVC" errorMessage=""> | ||
| <input | ||
| inputMode="numeric" | ||
| maxLength={3} | ||
| value={inputValue} | ||
| onChange={(e) => handleChange(e.target.value)} | ||
| css={[ | ||
| baseInputStyle, | ||
| css` | ||
| border: 1.01px solid #acacac; | ||
| `, | ||
| ]} | ||
| placeholder={"123"} | ||
| /> | ||
| </InputSectionLayout> |
There was a problem hiding this comment.
CVC 섹션에서 검증 에러가 노출되지 않습니다
InputSectionLayout에 errorMessage=""를 고정 전달하고 있어 유효성 실패가 화면에 표시되지 않습니다. 또한 blur 시점 검증도 연결되어 있지 않아, 현재 구현으로는 CVC 오류 피드백을 줄 수 없습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/components/CardForm/CvcInputSection/CvcInputSection.tsx` around
lines 16 - 30, The CVC input never shows validation because CvcInputSection
passes a hardcoded errorMessage="" to InputSectionLayout and lacks blur-time
validation; update CvcInputSection to (1) derive an error string from the
existing validation state/validator (e.g., validateCvc or cvcError state) and
pass that value into the InputSectionLayout errorMessage prop instead of "" and
(2) wire an onBlur handler on the <input> that triggers the same validation used
in handleChange (e.g., call handleBlur or validateCvc) so errors are computed
and displayed on blur as well as change; reference InputSectionLayout,
errorMessage, CvcInputSection, handleChange, and add an onBlur that updates the
validation state.
| }: { | ||
| children?: React.ReactNode; | ||
| title: string; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1) tsconfig의 타입 제한 확인
rg -n '"types"\s*:\s*\[' my-app/tsconfig.app.json -C2
# 2) React 네임스페이스 타입 참조 사용 위치 확인
rg -n '\bReact\.ReactNode\b' my-app/src -C2
# 3) 동일 파일 내 react 타입 import 유무 확인
rg -n 'from "react"|from '\''react'\''' my-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.tsx -C2Repository: woowacourse/react-payments
Length of output: 1122
🏁 Script executed:
cat -n my-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.tsxRepository: woowacourse/react-payments
Length of output: 3242
🏁 Script executed:
head -30 my-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.tsxRepository: woowacourse/react-payments
Length of output: 578
🏁 Script executed:
cat -n my-app/tsconfig.app.jsonRepository: woowacourse/react-payments
Length of output: 910
🏁 Script executed:
# Find all React namespace usages
rg 'React\.\w+' my-app/src --type ts --type tsx | head -20Repository: woowacourse/react-payments
Length of output: 98
🏁 Script executed:
rg 'import.*React.*from.*["\']react["\']' my-app/src --type ts --type tsx | head -10Repository: woowacourse/react-payments
Length of output: 139
🏁 Script executed:
# Find all React namespace usages without file type restriction
rg 'React\.\w+' my-app/src | head -20Repository: woowacourse/react-payments
Length of output: 593
🏁 Script executed:
# Check for explicit React imports
rg 'import.*React' my-app/src | grep "from.*react" | head -15Repository: woowacourse/react-payments
Length of output: 52
🏁 Script executed:
cat -n my-app/tsconfig.jsonRepository: woowacourse/react-payments
Length of output: 238
🏁 Script executed:
cat -n my-app/vite.config.tsRepository: woowacourse/react-payments
Length of output: 1849
🏁 Script executed:
# Check what vite/client provides
rg 'declare.*React|namespace React' my-app/node_modules/vite/client.d.ts 2>/dev/null || echo "Check if React types available through vite"Repository: woowacourse/react-payments
Length of output: 114
🏁 Script executed:
# Check package.json dependencies
cat my-app/package.json | grep -A 50 '"dependencies"'Repository: woowacourse/react-payments
Length of output: 1190
🏁 Script executed:
rg 'export.*React' my-app/node_modules/@emotion/react 2>/dev/null | head -10 || echo "Checking emotion react"Repository: woowacourse/react-payments
Length of output: 52
🏁 Script executed:
ls -la my-app/node_modules/@types/ 2>/dev/null | grep react || echo "No `@types/react` found"Repository: woowacourse/react-payments
Length of output: 92
React.ReactNode 타입 참조가 TypeScript 설정으로 인해 컴파일 실패를 야기합니다.
tsconfig.app.json에서 "types": ["vite/client"]로 설정되어 있어 @types/react가 타입 해석에 포함되지 않습니다. 따라서 Line 26의 React.ReactNode 참조는 Cannot find namespace 'React' 컴파일 에러를 발생시킵니다.
import type { ReactNode } from "react" 형태로 명시적으로 임포트하여 해결하세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/components/CardForm/InputSectionLayout/InputSectionLayout.tsx`
around lines 25 - 27, The component's props currently reference React.ReactNode
which fails under the project's TS config; update the top of
InputSectionLayout.tsx to add an explicit type import—import type { ReactNode }
from "react"—and change the props signature to use ReactNode instead of
React.ReactNode (the symbol to edit is the props object in the
InputSectionLayout component where children?: React.ReactNode is declared).
| <ul | ||
| css={css` | ||
| position: absolute; | ||
| top: 36px; | ||
| left: 0; | ||
| width: 100%; | ||
| border: 1px solid #acacac; | ||
| border-radius: 5.31px; | ||
| background: #fff; | ||
| list-style: none; | ||
| margin: 0; | ||
| padding: 0; | ||
| z-index: 10; | ||
| max-height: 247.55px; | ||
| overflow-y: auto; | ||
| `} | ||
| > | ||
| {options.map((option) => ( | ||
| <li | ||
| key={option} | ||
| onClick={() => handleSelect(option)} | ||
| css={css` | ||
| padding: 8px 12px; | ||
| font-size: 10.63px; | ||
| cursor: pointer; | ||
| color: #4f4f4f; | ||
| background: ${value === option ? "#f3f3f3" : "#fff"}; | ||
| font-weight: ${value === option ? 600 : 400}; | ||
| &:hover { | ||
| background: #f3f3f3; | ||
| } | ||
| `} | ||
| > | ||
| {option} | ||
| </li> | ||
| ))} |
There was a problem hiding this comment.
옵션 리스트가 키보드 접근성을 충족하지 못합니다.
현재 옵션이 li + onClick 기반이라 키보드 포커스/선택 흐름(화살표 이동, Enter 선택)이 보장되지 않습니다. 카드사 선택은 필수 입력 경로라서 접근성 관점에서 차단 이슈가 됩니다. button/role="option" + 키보드 이벤트, 또는 네이티브 select에 준하는 상호작용 보장이 필요합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/components/Select/Select.tsx` around lines 52 - 87, The option
list rendered by options.map inside the Select component (the <ul> with <li>
that calls handleSelect) is not keyboard-accessible; change each option from a
plain clickable <li> to an accessible, focusable control (either a <button> or
an element with role="option" and tabindex={0}) and implement keyboard handlers:
add an internal activeIndex state and onKeyDown on the list (or each option) to
support ArrowUp/ArrowDown to move focus/activeIndex and Enter/Space to call
handleSelect(option); also expose proper ARIA attributes (aria-selected or
aria-activedescendant on the list and role="listbox"/role="option") and keep the
existing visual highlight logic using value or activeIndex so keyboard users can
navigate and select options.
|
|
||
| export const detectBrand = (number: string): string => { | ||
| if (/^4/.test(number)) return "visa"; | ||
| if (/^5[1-5]/.test(number)) return "master"; |
There was a problem hiding this comment.
MasterCard 감지 범위가 누락되어 정상 카드가 거절될 수 있습니다.
Line 15의 ^5[1-5]만으로는 2221–2720 BIN 대역을 포함하지 못합니다. 현재 로직이면 2-series MasterCard가 detectBrand에서 누락되어 유효성 검사 실패로 이어집니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/constants/cardBrand.ts` at line 15, The MasterCard detection only
checks /^5[1-5]/ so it misses the 2221–2720 BIN range; update detectBrand to
also detect 2-series MasterCards by checking the first 4 digits numerically
(e.g., parseInt(number.slice(0,4),10) between 2221 and 2720) or replace the
regex with one that covers both ranges, and return "master" when either
condition matches (update the condition currently using /^5[1-5]/ in the
detectBrand implementation).
| const companyHandler = (company: string) => { | ||
| setCardInfo((prev) => ({ ...prev, company })); | ||
| setStep((prev) => Math.max(prev, 2)); | ||
| }; |
There was a problem hiding this comment.
회사명 입력 시 단계 진행 로직을 다시 살펴보세요.
36-39번 줄의 companyHandler는 company 값에 관계없이 항상 step을 2로 진행시킵니다. 하지만 68번 줄의 isCompanyValid는 빈 문자열을 유효하지 않은 것으로 판단하고 있습니다.
질문:
- 사용자가 회사명을 입력하지 않고 다음 단계로 넘어갈 수 있어야 할까요?
- 만약 필수 입력이라면, handler에서 단계를 진행시키기 전에 어떤 검증을 추가해야 할까요?
cardNumberHandler(31-33번 줄)와expiryHandler(43-45번 줄)에서는 검증 후 단계를 진행시키는데, 이 패턴을companyHandler에도 적용할 수 있을까요?
힌트: 다른 handler들은 입력이 완료되고 유효한지 확인한 후에 Math.max로 단계를 진행시킵니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/hooks/useCardForm.ts` around lines 36 - 39, companyHandler
currently advances the step to 2 unconditionally; change it to validate the
input (use the existing isCompanyValid check or ensure company !== ''/meets
validation rules) before calling setStep, mirroring the pattern used in
cardNumberHandler and expiryHandler: first update state via setCardInfo, then if
the value passes validation call setStep(prev => Math.max(prev, 2)), otherwise
do not advance (and optionally keep existing error state handling).
| const { state } = useLocation(); | ||
| const navigate = useNavigate(); | ||
| const numbers: string[] = state?.numbers ?? []; | ||
| const brand: string = state?.brand ?? ""; |
There was a problem hiding this comment.
라우트 state 누락 시 완료 페이지가 깨집니다.
Line 7-10, Line 39-40에서 /complete 직접 진입(또는 새로고침) 시 numbers[0]/brand가 비어 문구가 비정상적으로 렌더링됩니다. state 유효성 체크 후 없으면 /로 리다이렉트하거나 안전한 대체 UI를 먼저 렌더링하세요.
Also applies to: 39-40
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/pages/CardComplete.tsx` around lines 7 - 10, The CardComplete
component currently assumes route state exists and directly reads numbers[0] and
brand, which breaks when a user lands on /complete via refresh or direct link;
update the component to validate the state from useLocation() (symbols:
useLocation, useNavigate, state, numbers, brand) and if numbers is empty or
brand is falsy, perform a safe redirect using navigate('/') or render a fallback
UI (e.g., a loading/empty state) instead of rendering numbers[0]/brand; ensure
this validation runs early (on mount or before JSX render) so references to
numbers[0] and brand are never used when undefined.
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
| if (!isValid) return; | ||
| navigate("/complete", { state: { cardInfo, brand } }); | ||
| }; |
There was a problem hiding this comment.
민감 결제정보를 라우트 state로 직접 전달하지 마세요.
cardInfo에 카드번호/CVC/비밀번호가 포함된 채 /complete로 전달되고 있습니다. 완료 페이지에는 마스킹된 표시값(예: 브랜드, 마지막 4자리) 또는 서버 토큰만 넘기고, 원문 민감정보는 즉시 폐기하는 구조로 바꾸는 것이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@my-app/src/pages/CardForm.tsx` around lines 27 - 31, The handleSubmit
currently navigates to "/complete" passing the full cardInfo (including
PAN/CVC/password) in route state; change handleSubmit to remove sensitive fields
before navigation by either (a) generating a masked object (e.g., maskedCardInfo
containing brand and last4) or (b) sending cardInfo to a tokenization function
(e.g., tokenizeCard or submitCardToServer) and passing only the server token or
masked result; update the call to navigate("/complete", { state: { /*
maskedCardInfo or token */ , brand } }) and ensure the original cardInfo is
cleared/discarded after tokenization to avoid keeping plaintext secrets in
memory/state.
degurii
left a comment
degurii
left a comment
There was a problem hiding this comment.
안녕하세요 포도. 리뷰가 늦어서 정말 죄송합니다.
디스크립션에 적어주신 학습 흐름과 질문에 답변 정리해드려쏙, 인라인 코멘트도 몇 개 따로 남겨두었어요. 같이 확인 부탁드립니다.
이번 단계 진행하시느라 고생 많으셨습니다!
1) 학습 경험에 대한 코멘트
1. 훅 분리 기준
저번 1단계 리뷰에서 폼 상태를 더 본격적으로 다루게 되면 hook 분리 기준을 다시 마주칠 거라고 말씀드렸는데, 이번에 액션 플랜으로 설정해주신 것처럼 분리 기준을 논리적으로 정리하시고, 실제로 적용까지 해주신 부분이 좋았습니다. 단순히 재사용 가능한 로직이라는 단일 기준만으론 단일 사용 hook의 정당화가 어렵다는 걸 직접 체감하셨고, 거기서 새로운 기준을 만들어가신 게 학습 단계에서 의미가 큰 경험이라 생각합니다.
단일 사용 hook이 이 규모에서 의미를 가질 수 있는지에 대한 의문에는 재사용 빈도 외에 어떤 책임을 한 곳에 모으는가의 관점이 있다고 답변드릴 수 있을 것 같습니다. 이번 코드에서 제가 생각한 훅 분리에 따른 이점은 다음과 같을 것 같아요.
- 페이지 컴포넌트가 조립에만 집중하게 두기
: CardForm 페이지가 useCardForm을 안 썼다면 87줄 로직 + 5개 핸들러 + step 관련 JSX를 한 페이지가 다 들고 있었을 거에요. hook으로 분리하면 페이지는 어떤 섹션을 어떤 순서로 보여줄지에 대한, UI와 비즈니스 로직의 오케스트레이션 책임에 가까워집니다. - 같이 변할 책임을 한 곳에 묶기
:cardInfo상태,brand/fieldConfig파생,step같은 값들이 동기화되어야 하는데, 이걸 페이지 안에 흩어두면 변경이 여러 곳에 새기 쉽습니다.
다만 단일 사용이라는 점에서 페이지 안에 두고 필요해질 때 분리해도 늦지 않다는 시각도 합리적입니다. 이건 사람마다, 팀마다 다 다르게 가치를 두는 부분이에요. 그러니 어느 쪽이 더 맞다보다는 본인이 세운 기준과 이를 드러내는 코드가 잘 일치하기만 한다면 충분히 괜찮다고 생각합니다.
2. 커서 동적 이동
useEffect는 렌더링 이후에 실행되므로 포커스 이동 타이밍이 미묘하게 어긋날 수 있고, 개념적으로도 사용자 액션에 대한 즉각적인 반응보다 렌더링의 부수 효과에 가깝다는 점이 맞지 않는다고 판단했습니다.
요렇게 적어주셨는데, 제 생각도 동일합니다. 이걸 의식적으로 구분해서 합리적으로 결론을 내린 부분이 인상깊었어요.
2) 논의하고 싶은 부분에 대한 답변
1. 훅 분리의 적절성
useInputValidation과 useCardForm을 따로 짚어드릴게요.
useInputValidation: 지난 1단계 리뷰에서 blur 검증을 한번에 hook으로 묶기 전에 검증 함수만 먼저 빼보라고 안내드렸는데, 그부분이 잘 반영된 형태로 보여요. validator를 인자로 받고 상태/이벤트만 관리하니까 도메인 검증과 hook의 책임이 깔끔하게 갈라져 있고, 네 곳에서 같은 인터페이스로 쓰이고 있어 재사용성도 잘 챙기셨다고 생각합니다.
이슈는 아니지만, 훅이 반환하는 errorIndex가 단일 input 섹션(Cvc, Password)에선 사용되지 않고 있거든요. 모든 사용처에 맞춰 인터페이스를 통일했다고 볼 수도 있기 때문에 문제되지는 않지만, 규모가 커지는 경우 단일/다중을 분리한 hook으로 나누는 방향도 고려 가능한 선택지 중에 하나일 수 있어 간단히 언급만 드립니다. (절대 수정 제안의 의도가 아닙니다!)
useCardForm: 폼 진행 전체를 한 곳에 모은 형태입니다. 위에서 말씀드린 페이지 셸 분리 관점에서 분리가 자연스러운 결정이고, 같이 변할 책임도 한 곳에 모여 응집도도 좋다고 생각합니다.
다만 cardNumberHandler 하나가 여러 일을 하고 있는 점은 짚어볼만 할 것 같아요. brand 재계산, 입력값 길이 재조정, cardinfo state 셋팅, step 셋팅 이렇게 네가지 일을 한번에 처리하고있거든요. 핸들러라는 특성상 자연스러운 구성이긴 하지만, 각 로직마다 if분기와 같은 판단의 책임까지 함께 붙어있어서 조금 더 책임에 맞게 로직들을 간단한 함수로 분리하고, 이를 핸들러에서 순서에 맞게 호출해서 컨트롤하는 흐름도 괜찮을 것 같다는 생각도 들어요.
물론 이런 부분들을 무리해서 다 분리하는 건 오히려 복잡도를 늘리는 방향이 될 수 있거든요. 그러니 지금 단계에선 이런 로직들이 한곳에 응집되어 있는 편이 더 자연스러운지, 아니면 책임이 분리된 작은 단위들의 조합이 더 자연스러운지 한번쯤 생각만 해봐도 좋을 것 같습니다.
2. 유사한 구조의 컴포넌트 통합 여부
제가 평소 선호하는 방식과 완전히 일치하는 결정이라, 개인적으론 그대로 가져가셔도 될 것 같습니다.
같은 모양이라는 이유로 합치기 시작하면 props 분기가 늘어나고, 한쪽 컴포넌트의 스펙이 바뀌면 다른 쪽까지 영향을 받는 구조가 됩니다. 도메인적으로 서로 다른 책임을 가진 컴포넌트는 코드가 비슷해 보여도 같은 이유로 변하지 않는다는 점에서 분리된 채로 두는 게 변경에 유연합니다.
3. 비밀번호 마스킹 처리 범위
조직마다, 회사마다 다를텐데, 저희는 보통 기획 주체가 누구냐로 답이 갈립니다.
저희 환경 기준으로 말씀드리면, 지금같은 상황에 PM 기획자분에게 먼저 이야기를 꺼내보는 편이에요. 현재 상황의 문제를 공유하면 기획자가 디자인 의도가 어디까지인지 판단해서 방향을 결정해줍니다. 이때 기술적으로 비용이 많이 든다는 정보를 함께 전달하면 보통 type=password 같은 표준 도구로 가는 쪽으로 합의되는 편이기는 해요. 다만 디자인 일관성이 명확히 의도된 부분이라면 커스텀 구현으로 갈 수도 있구요.
이번 미션처럼 기획자가 따로 없는 환경에선 본인이 그 판단까지 같이 하셨어야 했을텐데, 적절한 판단하에 잘 결정해주신 것 같습니다.
4. 섹션이 늘어날수록 길어지는 유효성 검사 체인
코드 코멘트로도 비슷한 결의 의견을 남겼는데, 여기에 한 번 더 정리하겠습니다.
구조적 리팩토링을 고려하는 타이밍은 실제 유지보수에서 불편함이 비용으로 나타날 때라고 생각합니다. 지금 5개 필드에서 isXxxValid 변수와 && 체인이 늘어나는 건 가독성 측면에선 큰 문제는 아니에요. 다만 학습 과정이니까 시간이 된다면 형식화된 구조로 한번 바꿔보시는 것도 좋은 경험이 될 것 같습니다.
큰 관점에서의 구조를 구현하지는 않아도 될 것 같고, 간단히 생각해볼 수 있는 방향 옵션을 몇 가지 드리면
- 단순 객체로 관리:
Object.values(validations).every(Boolean) - 필드 메타데이터 배열 + Map:
fields.every(name => validatorMap[name](cardInfo[name])) - 현재 구조 유지
세 방향이 다 트레이드 오프가 있어요. 객체/map 기반은 필드 추가가 가벼워지지만 각 필드의 유효조건이 명시적으로 드러난다는 가독성을 잃습니다. 하지만 지금처럼 명시적 변수는 5개에선 자연스럽지만, 필드가 많아질수록 부담이 됩니다.
본인이 어느 시점에 이건 좀 줄여야겠다는 느낌이 들 때 옮기시면 자연스러운 타이밍일 거예요. 미리 구조화하면 오히려 가독성이나 복잡도 면에서 더 손해를 볼 수도 있으니까요.
| const isCompanyValid = cardInfo.company !== ""; | ||
| const isPasswordValid = cardInfo.password.length === 2; | ||
|
|
||
| const isValid = isNumbersValid && isCompanyValid && isExpiryValid && isCvcValid && isPasswordValid; |
There was a problem hiding this comment.
[제안]
현재 규모에서는 5개정도로 끝이지만, 이후에 관련 필드가 점점 늘어나는 경우를 상정해봐도 좋을 것 같아요.
만약 10개에 대해 동일한 처리를 해야한다면 지금 구조로 괜찮을지, 아니면 어떻게 개선해볼 수 있을지 생각해보면 좋을 것 같습니다.
| {children} | ||
| </form> | ||
| </div> | ||
| {isValid && ( |
There was a problem hiding this comment.
[소소한 제안] README에는 "버튼 활성화/비활성화"로 적혀 있는데, 코드는 isValid && <button>으로 노출 여부 자체를 토글하고 있어요. 두 동작은 사용자 경험이 다른데, readme와 코드를 의도에 맞게 한쪽으로 맞춰주시면 좋을 것 같습니다.
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
| if (!isValid) return; | ||
| navigate("/complete", { state: { numbers: cardInfo.numbers, brand } }); |
There was a problem hiding this comment.
[권장] CardComplete가 useLocation().state에 전적으로 의존해서, 직접 URL 입력이나 새로고침 시 state가 사라지면 폴백 값이 들어가 "0123로 시작하는 카드가 등록되었어요" 같은 깨진 문구가 나올 수 있을 것 같아요.
router state는 휘발성이라 페이지 진입 보장 매체로 쓰기 어려운 점을 알아두시면 좋을 것 같습니다. state 부재 시 home 리다이렉트하거나 정상 화면을 조건부로 렌더링하는 형태면 좀 더 안전할듯해요.
추가로 cardInfo에서 민감 정보들을 잘 빼고 보내셨는데, 화면에서 numbers[0]만 쓰니까 numbers 배열 전체보다 정말 필요한 값만 좁혀 넘긴다면 보안 측면에서도 더 좋을 것 같습니다.
3 participants
🎯 페이먼츠
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
2단계
3단계
idle | loading | success | error네 가지로 명시적으로 관리한다.🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 동키콩!
이번 미션 쉽지 않았지만 열심히 고민하며 구현해봤습니다!
아직 코드에서 마음에 들지 않는 부분도 많고, 더 개선하고 싶은 지점도 많이 남아 있습니다.
그래서 가감 없는 피드백을 주시면 정말 감사하겠습니다 🤩
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
1. 훅 분리의 기준
처음에는 재사용 가능한 로직이라는 기준 하나만 가지고 훅 분리를 판단했는데, 그것만으로는
useCardForm처럼 한 곳에서만 쓰이는 훅을 왜 분리해야 하는지 납득하기 어려웠습니다.고민 끝에 재사용성 외에 다음 기준들을 추가로 세우니 분리의 필요성이 더 명확하게 보이기 시작했습니다.
다만 이 기준을 안다고 해서 실제 적용이 쉬운 건 아니었습니다. 무조건 훅으로 빼면 오히려 파일이 늘고 흐름을 따라가기 어려워지기 때문에, 이번에는 우선 두 가지만 분리해보았습니다.
useCardForm-CardForm.tsx에서 JSX보다 5개 핸들러, step 전진 조건, 브랜드 감지, 유효성 판단 로직이 훨씬 많아졌고, 카드 입력 전체 흐름 관리라는 하나의 역할이 명확해 보여서 분리하게 되었습니다.useInputValidation- blur에서 검증, change에서 에러 초기화라는 단일 흐름이 명확한 데다,CardNumberInputSection,ExpiryDateInputSection,CvcInputSection,PasswordInputSection네 곳에서 동일하게 반복되고 있어 재사용성 면에서도 분리할 만하다고 느꼈습니다.그럼에도 솔직히 말하면
useCardForm은CardForm.tsx한 곳에서만 사용되고 87줄 규모라서, 분리의 필요성을 몸으로 체감하기는 어려웠습니다. 실제로 이 훅을 만든 직접적인 동기는 학습 목표인 custom hook으로 Form 관리 로직을 컴포넌트에서 분리한다라는 요구사항이었습니다. 단일 사용 훅이 이 규모에서도 의미를 갖는 다른 기준이 있는지 의문이 남았습니다.2. 카드 번호 입력 칸 수의 동적 제어
이번 미션에서 제일 까다로웠던 부분이 카드 번호 입력이었습니다. 브랜드마다 번호 자릿수와 칸 구성이 다른 데다 사용자가 입력하는 도중에 실시간으로 브랜드를 감지해야 했기 때문입니다. Visa·Master는 4-4-4-4, Amex는 4-6-5, Diners는 4-6-4로 칸 수 자체가 다릅니다. 즉 어떤 카드인지 모르는 상태에서 입력을 받기 시작해야 하고, 입력 도중 브랜드가 확정되면 그 순간 UI도 함께 바뀌어야 한다고 판단했습니다.
이를 해결하기 위해
BRAND_FIELD_CONFIG라는 상수로 브랜드별 필드 구성을 관리하고,detectBrand로 감지한 브랜드를getFieldConfig에 넘겨 현재 렌더링할 칸 수를 결정하는fieldConfig를 도입했습니다.useCardForm내부에서 브랜드가 바뀌어 칸 수가 달라지면 기존 입력값 배열도 함께 조정합니다.다만 이 방식에는 브랜드가 바뀌는 순간 기존에 입력한 값 일부가 잘릴 수 있다는 단점이 있습니다. 이를 막기 위해 초기에는 앞 2개의 필드만 열어두고, 브랜드가 확정된 이후에 나머지 칸을 펼치는 방식도 고려했습니다. 그러나 카드 번호 입력 초기(8자리 미만)에만 브랜드 판정이 이루어지므로 실제로 잘리는 상황은 거의 발생하지 않아 우선은 현재 방식을 유지했습니다.
3. 커서의 동적 이동
사용자 편의를 위해 포커스를 이동시키는 부분에서도 고민이 많았습니다. 각 칸이 꽉 찼을 때 자동으로 다음 칸으로 포커스를 이동시키는 UX를 구현하면서,
useEffect로 상태 변화를 감지하는 방식과useRef로 DOM에 직접 접근하는 방식 사이에서 고민했습니다.결론적으로
useRef를 사용했습니다. 포커스 이동은 상태 변화의 결과가 아니라 사용자 입력 이벤트의 직접적인 반응이기 때문에, 이벤트 핸들러 안에서 즉시 처리하는 것이 더 자연스럽다고 느꼈습니다.useEffect는 렌더링 이후에 실행되므로 포커스 이동 타이밍이 미묘하게 어긋날 수 있고, 개념적으로도 사용자 액션에 대한 즉각적인 반응보다 렌더링의 부수 효과에 가깝다는 점이 맞지 않는다고 판단했습니다.여기서
fieldConfig가 동적으로 바뀐다는 점이 ref 초기화 방식에도 영향을 미쳤습니다.useRef([null, null, null, null])처럼 고정 배열로 초기화하면, 예를 들어 Amex로 브랜드가 감지되어 칸이 3개로 줄었을 때 inputRefs.current[3]에 이전 ref가 그대로 남아 있게 됩니다. ref callback 방식을 사용하면 이 문제가 자동으로 해결되는데, React는 DOM 요소가 마운트될 때 해당 요소를, 언마운트될 때 null을 콜백 인자로 넘겨주기 때문에 fieldConfig가 바뀌어 input이 사라지면 해당 인덱스의 ref도 null로 정리된다는 것을 이번에 배웠습니다.덕분에 포커스 이동 로직에서
inputRefs.current[index + 1]?.focus()를 호출할 때, 현재 실제로 렌더링된 input만 정확하게 참조할 수 있었습니다.2) 이번 리뷰를 통해 논의하고 싶은 부분
훅 분리의 적절성
위에서 언급한 기준으로 useCardForm과 useInputValidation 두 가지를 분리했는데, 이 분리가 적절했는지 궁금합니다. 더 분리할 만한 로직이 있는지, 혹은 반대로 불필요한 분리가 있는지 의견을 듣고 싶습니다.
유사한 구조의 컴포넌트 통합 여부
PasswordInputSection과 CardNumberInputSection이 구조적으로 비슷해서 SingleInputSection/MultiInputSection처럼 타입을 분리하거나, inputs prop으로 단일/배열을 모두 받는 컴포넌트로 통합하는 방식을 고려했습니다. 다만 두 컴포넌트의 책임이 달라 억지로 합치면 오히려 props 분기가 늘어나고, 현재 코드량도 많지 않아 분리된 채로 두는 것이 낫다고 판단했습니다. 같은 구조의 컴포넌트가 더 늘어날 때 추상화해도 늦지 않다고 생각해 우선은 그대로 두었는데, 이 판단에 대한 의견이 궁금합니다.
비밀번호 마스킹 처리 범위
피그마 디자인에서는 비밀번호 입력 시
**로 마스킹되도록 되어 있었습니다. type="password"로 처리하면 브라우저 기본 동작(••)과 차이가 생기는데, 이를 커스텀 마스킹으로 완전히 맞추려면 구현 비용이 상당하다 판단해 이번에는 브라우저 기본 동작에 맡겼습니다. 현업에서는 이런 디자인 디테일까지 반드시 구현해야 하는지, 혹은 어느 선에서 타협하는 게 일반적인지 궁금합니다.섹션이 늘어날수록 길어지는 유효성 검사 체인
현재 useCardForm에서 필드가 하나 추가될 때마다 isXxxValid 변수 하나와 isValid 체인에 && 하나씩 늘어나는 구조입니다. 필드가 계속 추가된다면 관리가 번거로워질 것 같다고 생각합니다. 이 정도 규모에서는 현재 방식을 유지하는 게 나은지, 아니면 더 나은 구조가 있는지 의견이 궁금합니다.
✅ 리뷰어 체크 포인트
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*를 사용했는가?