[페이먼츠 2단계 - hooks & state] 벤지(김현중) 미션 제출합니다.#528
Open
hjkim0905 wants to merge 60 commits into
Open
Conversation
… Validator to collect related logic together
…f statement in changeCardNumber()
…licate brand detection logic
…nd fix stories to use array for cardNumber
…m is not completed
|
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은 카드 결제 폼을 다단계 프로세스로 확장합니다. CardContext 구조를 배열 기반으로 재설계하고, 폼 로직을 커스텀 훅으로 분리하며, React Router를 통한 라우팅을 추가합니다. 검증 로직은 예외 던지기에서 ValidationResult 객체 반환으로 변경되고, UnionPay를 포함한 5가지 카드 브랜드 감지가 구현됩니다. CardPasswordInput, CardSelectionDropdown, ConfirmButton이 새로 추가되고, 카드 미리보기는 카드사별 배경색 변경을 지원합니다. 예상 코드 리뷰 노력🎯 4 (복잡함) | ⏱️ ~45분 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🧹 Nitpick comments (8)
app/src/components/preview/CardNetworkBrand.tsx (1)
10-16: ⚡ Quick win
selectBrandImage함수의 완전성을 확인해보세요.현재 함수가
undefined를 반환할 수 있어서<img src={undefined}>가 될 수 있습니다.NetworkBrand타입에 새로운 브랜드가 추가되었지만 이 함수가 업데이트되지 않으면 어떻게 될까요?생각해볼 점:
- TypeScript의 exhaustiveness checking을 활용할 수 있을까요?
- switch 문으로 변경하고 default case에서 에러를 던지면 어떨까요?
- 마지막에 기본 이미지를 반환하는 것은 어떨까요?
타입 안전성을 높이는 방법을 고민해보세요.
🤖 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 `@app/src/components/preview/CardNetworkBrand.tsx` around lines 10 - 16, selectBrandImage currently can return undefined (leading to <img src={undefined}>) and won't fail if NetworkBrand gains new values; change it to use a switch on the brand (function selectBrandImage) and handle every known NetworkBrand case, and add an exhaustive default that either throws an Error or returns a safe fallback image; for full type-safety use a never-check in the default branch (e.g., const _exhaustive: never = brand) so TypeScript will force you to update selectBrandImage when NetworkBrand changes.app/src/hooks/useCardForm.ts (1)
19-20: ⚡ Quick win네트워크 브랜드별 길이 로직의 위치를 고민해보세요.
현재
lastDigitLength계산 로직이 hook 안에 하드코딩되어 있습니다. 이런 브랜드별 규칙이 여러 곳에서 필요하다면 어디에 위치하는 것이 좋을까요?몇 가지 생각해볼 점:
BrandValidator에getExpectedLength(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 `@app/src/hooks/useCardForm.ts` around lines 19 - 20, The hook useCardForm currently hardcodes brand-specific digit lengths in the lastDigitLength calculation using BrandValidator.detectNetworkBrand(...) which reduces reuse and makes maintenance harder; move this logic into BrandValidator by adding a method like getExpectedLastDigits(brand) or getExpectedLength(brand) (or export a BRAND_LENGTHS constant) and replace the inline ternary in useCardForm with a call to that new API (referencing BrandValidator and lastDigitLength in the hook) so all brand rules are centralized and reusable across the app.app/src/validators/CardValidator.ts (1)
56-65: ⚡ Quick win만료일 검증 로직을 점검해보세요.
현재 구현에 대해 몇 가지 생각해볼 점이 있습니다:
중복 코드: Line 42와 58에서 현재 연도를 계산하는 로직이 중복됩니다. 이를 어떻게 개선할 수 있을까요?
2자리 연도의 한계: 현재 2자리 연도를 사용하고 있습니다(예: 26 = 2026). 2100년 이후에는 어떻게 될까요?
- 신용카드 유효기간이 보통 짧아서 실무적으로 문제가 없을 수 있습니다
- 하지만 이런 가정을 코드나 문서에 명시하는 것이 좋을까요?
검증 순서:
isValidYear가 먼저 호출되고 나서isValidCardExpiryDate가 호출되나요? 각 검증기의 책임 범위를 어떻게 나누는 것이 좋을까요?이런 엣지 케이스와 코드 구조에 대해 팀과 논의해보세요.
🤖 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 `@app/src/validators/CardValidator.ts` around lines 56 - 65, Refactor isValidCardExpiryDate to remove duplicated current-year logic by reusing a single computed currentYear/currentMonth (extract into a helper or use existing isValidYear flow), ensure the function handles two-digit years deterministically (either convert 2-digit input to full year using a defined pivot or validate/explicitly document the two-digit-year assumption), and make sure isValidYear is called before isValidCardExpiryDate so expiry check only runs after year/month format and range are validated; update references to isValidCardExpiryDate and isValidYear accordingly so responsibilities are clear and duplication eliminated.app/src/components/preview/CardPreview.tsx (1)
11-11: ⚡ Quick win
networkBrand계산을 메모이제이션하세요.
BrandValidator.detectNetworkBrand(cardNumber.join('')).brand가 매 렌더링마다 실행됩니다.cardNumber가 변경될 때만 재계산되도록useMemo를 사용하는 것이 좋습니다.♻️ 제안된 개선 사항
+import { useMemo } from 'react'; + export function CardPreview() { const { cardNumber, cardCompany } = useCardContext(); - const networkBrand = BrandValidator.detectNetworkBrand(cardNumber.join('')).brand; + const networkBrand = useMemo( + () => BrandValidator.detectNetworkBrand(cardNumber.join('')).brand, + [cardNumber] + );🤖 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 `@app/src/components/preview/CardPreview.tsx` at line 11, Memoize the networkBrand calculation so BrandValidator.detectNetworkBrand(cardNumber.join('')).brand only runs when cardNumber changes: wrap that expression in a useMemo (imported from React) and use cardNumber.join('') as the dependency (or the cardNumber array if you prefer) so re-computation occurs only when the card number changes; update the reference to networkBrand inside the component to use the memoized value.app/src/components/form/CardForm.stories.tsx (1)
45-45: 💤 Low value완성된 폼 상태를 보여주는 스토리를 추가하면 좋겠습니다.
현재
isFormComplete가false로 고정되어 있어ConfirmButton이 항상 비활성화 상태로 표시됩니다. 폼이 완성되었을 때의 UI 상태를 확인할 수 있도록isFormComplete: true이고 모든 필드가 채워진 별도의 스토리를 추가하는 것을 고려해보세요.🤖 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 `@app/src/components/form/CardForm.stories.tsx` at line 45, Add a new story in CardForm.stories that represents the completed form state: create a separate story (e.g., "Completed" or "FormComplete") that sets isFormComplete: true and supplies sample values for all required fields used by CardForm (same props used in the existing story), so ConfirmButton renders enabled; reference the existing story pattern and props (isFormComplete, the CardForm story name and prop shape) to mirror structure and only change isFormComplete and field values.app/src/components/form/CardNumberInput.stories.tsx (2)
15-18: 💤 Low value메타에 정의된 args가 사용되지 않습니다.
15-18번 라인에서
firstRef와onComplete를 기본 args로 정의했지만, 24-46번 라인의renderWithContext함수는 이 args를 사용하지 않고 자체적으로 ref를 생성하고 빈 함수를 전달하고 있습니다.🤖 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 `@app/src/components/form/CardNumberInput.stories.tsx` around lines 15 - 18, The story defines default args firstRef and onComplete but renderWithContext ignores them and creates its own ref and noop; update renderWithContext in CardNumberInput.stories.tsx to use the passed-in args (args.firstRef and args.onComplete) instead of creating a new ref or inline empty function so the story harness and controls work correctly with the defined args (refer to renderWithContext, props.firstRef / onComplete and the args keys firstRef and onComplete).
52-60: ⚡ Quick win검증 시나리오에 기대값 단언이 누락되었습니다.
InvalidInput스토리는 비숫자 입력('abc')을 타이핑하고 탭을 이동하지만, 예상되는 동작에 대한 단언(assertion)이 없습니다. 다른 검증 스토리들(62-80번 라인)과 달리 이 스토리가 무엇을 테스트하려는지 명확하지 않습니다.어떤 검증 메시지나 동작을 기대하는지
expect문을 추가하는 것을 고려해보세요.🤖 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 `@app/src/components/form/CardNumberInput.stories.tsx` around lines 52 - 60, The InvalidInput Story's play function (rendered via renderWithContext) types non-numeric input into the first textbox (canvas.getAllByRole) and tabs away but lacks any assertion; add an expect after await userEvent.tab() to assert the validation outcome (e.g. expect a validation message to appear in the canvas or that the input has aria-invalid="true" or disabled submit behavior). Update the InvalidInput.play block to check for the specific error node or attribute your component emits (validation text content or aria-invalid) so the story clearly documents the expected failure mode.app/src/App.tsx (1)
7-12: ⚡ Quick win루트 경로("/")에 대한 라우트가 정의되지 않았습니다.
현재
/react-payments와/react-payments/complete만 정의되어 있어, 사용자가 루트 URL로 접속하면 빈 페이지가 표시됩니다.루트 경로를
/react-payments로 리다이렉트하거나, 별도의 랜딩 페이지를 고려해보세요. 또한 정의되지 않은 경로에 대한 404 페이지도 추가하면 사용자 경험이 개선됩니다.🤖 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 `@app/src/App.tsx` around lines 7 - 12, Add a root route and a fallback 404 route to the existing Router: inside BrowserRouter/Routes (where Route components for path="/react-payments" and "/react-payments/complete" are defined) add a Route for path="/" that redirects to "/react-payments" using Navigate from react-router-dom (or render a landing component if you prefer), and add a catch-all Route path="*" that renders a NotFound/404 component; ensure you import and reference Navigate and/or a NotFound component and update Route declarations (Card, RegistrationComplete) accordingly.
🤖 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 `@app/src/components/form/CardCVCInput.tsx`:
- Around line 32-37: The onChange handler in CardCVCInput currently calls
onComplete() solely based on length===3 which permits invalid inputs; modify the
handler in CardCVCInput.tsx to call onComplete() only when the value both has
length 3 and passes a CVC validation (e.g., a helper isValidCVC(value) or a
regex like /^\d{3}$/) so non-numeric or otherwise invalid 3-character inputs do
not trigger onComplete; locate the onChange arrow function and replace the
length-only check with a combined length+validation check (or add and reuse an
existing validateCVC function) before invoking onComplete().
In `@app/src/components/form/CardSelectionDropdown.tsx`:
- Around line 14-45: Add accessible behavior and external-click handling: attach
a ref to the dropdown wrapper/DropwdownBox and in a useEffect add/remove a
document mousedown/touchstart listener to close the menu when clicking outside
(use isDropdownOpen and setIsDropdownOpen). Add keyboard handling on
DropwdownBox and option elements to support ArrowUp/ArrowDown to move a focused
option index, Enter to select (calling setCardCompany and onSelect), and Escape
to close; manage focus with refs or a focusedIndex state. Add WAI-ARIA
attributes: on DropwdownBox set role="combobox" (or role="button") with
aria-haspopup="listbox" and aria-expanded={isDropdownOpen}; on OptionsBox set
role="listbox"; on each OptionItem set role="option" and
aria-selected={option.value === currentValue}. Finally visually indicate the
current selection by applying a selected style/class to the OptionItem when
option.value === the selected cardCompany.
In `@app/src/hooks/useCardForm.ts`:
- Around line 21-30: Replace the current length-only check in useCardForm's
isFormComplete (which inspects cardCompany, cardNumber[], lastDigitLength,
cardExpiryDate['expiry-month'|'expiry-year'], cardCVC, cardPassword) with actual
validation calls to the CardValidator API: call the card number validator for
the assembled number, the expiry validator for month/year, the CVC validator for
cardCVC, and the password validator for cardPassword (or equivalent isValid*
helpers exported from CardValidator.ts) and combine their boolean results so
isFormComplete reflects real validity rather than just string lengths.
In `@app/src/hooks/useCardNumberInput.ts`:
- Around line 48-59: The code mixes the stale render-time networkBrand with the
current input-derived brandResult.brand, causing inconsistent max-length and
completion checks; update the logic in useCardNumberInput (where
handleInputMaxLength is called and lastDigitLength is computed) to use
brandResult.brand consistently (instead of networkBrand) when computing
maxLength and lastDigitLength so that the focus/complete checks (involving
handleInputMaxLength(...).maxLength, lastDigitLength, and the cardNumber length
checks) all use the same brand source.
In `@app/src/hooks/useSingleInput.ts`:
- Line 19: Remove the sensitive console output from the useSingleInput hook by
deleting the console.log(value) call (the debug print inside useSingleInput) so
CVC/password inputs are not logged; if debugging is needed, replace with a
non-production-safe mechanism such as gated logging behind an explicit dev-only
flag or use secure dev-only breakpoints, but do not leave any console output of
the input value in the hook.
In `@app/src/page/RegistrationComplete.tsx`:
- Around line 9-17: The component reads firstDigits and cardCompany from
location.state and computes cardCompanyLabel, but doesn't handle missing state;
update the RegistrationComplete component to guard against null/undefined
location.state (or missing firstDigits/cardCompany): choose one
approach—redirect to the prior page when state is absent, or render a fallback
UI with default values/message—and implement it by checking location.state (and
the derived firstDigits/cardCompanyLabel) before rendering; use the existing
symbols (location.state, firstDigits, cardCompany, cardCompanyLabel,
cardCompanyOptions, and the RegistrationComplete component) to locate the logic
and add either a conditional early return (redirect) or conditional rendering
with a clear fallback message.
In `@app/src/validators/BrandValidator.ts`:
- Around line 8-13: The isUnionPay() regex incorrectly matches the 622126–622925
BIN block; update the first alternative in isUnionPay to precisely match 622126
through 622925 (keep the other two alternatives intact). Replace the current
/^622(1[2-9][6-9]|[2-9]\d{2})/ with a pattern that explicitly matches that
range, for example: ^622(?:12[6-9]|1[3-9]\d|[2-8]\d{2}|9[0-1]\d|92[0-5]) so the
function isUnionPay correctly validates 622126–622925 BINs.
In `@README.md`:
- Around line 42-43: 문서에서 중복된 섹션 제목 "## 기능 요구 사항"으로 인해 MD024 경고가 발생하므로 Step2 섹션의
헤더 텍스트를 고유하게 변경하세요; 파일 내 기존 헤더 "## 기능 요구 사항"과 충돌하는 두 번째 헤더(현재 "## 기능 요구 사항"으로
표기된 Step2)를 찾아 이름을 예를 들어 "## 기능 요구 사항 (Step2)" 또는 더 구체적인 "## 기능 요구 사항 — Step2:
세부사항"처럼 유니크한 텍스트로 바꾸고, 관련된 내부 링크나 참조가 있다면 동일한 새로운 헤더 텍스트로 함께 갱신하세요.
---
Nitpick comments:
In `@app/src/App.tsx`:
- Around line 7-12: Add a root route and a fallback 404 route to the existing
Router: inside BrowserRouter/Routes (where Route components for
path="/react-payments" and "/react-payments/complete" are defined) add a Route
for path="/" that redirects to "/react-payments" using Navigate from
react-router-dom (or render a landing component if you prefer), and add a
catch-all Route path="*" that renders a NotFound/404 component; ensure you
import and reference Navigate and/or a NotFound component and update Route
declarations (Card, RegistrationComplete) accordingly.
In `@app/src/components/form/CardForm.stories.tsx`:
- Line 45: Add a new story in CardForm.stories that represents the completed
form state: create a separate story (e.g., "Completed" or "FormComplete") that
sets isFormComplete: true and supplies sample values for all required fields
used by CardForm (same props used in the existing story), so ConfirmButton
renders enabled; reference the existing story pattern and props (isFormComplete,
the CardForm story name and prop shape) to mirror structure and only change
isFormComplete and field values.
In `@app/src/components/form/CardNumberInput.stories.tsx`:
- Around line 15-18: The story defines default args firstRef and onComplete but
renderWithContext ignores them and creates its own ref and noop; update
renderWithContext in CardNumberInput.stories.tsx to use the passed-in args
(args.firstRef and args.onComplete) instead of creating a new ref or inline
empty function so the story harness and controls work correctly with the defined
args (refer to renderWithContext, props.firstRef / onComplete and the args keys
firstRef and onComplete).
- Around line 52-60: The InvalidInput Story's play function (rendered via
renderWithContext) types non-numeric input into the first textbox
(canvas.getAllByRole) and tabs away but lacks any assertion; add an expect after
await userEvent.tab() to assert the validation outcome (e.g. expect a validation
message to appear in the canvas or that the input has aria-invalid="true" or
disabled submit behavior). Update the InvalidInput.play block to check for the
specific error node or attribute your component emits (validation text content
or aria-invalid) so the story clearly documents the expected failure mode.
In `@app/src/components/preview/CardNetworkBrand.tsx`:
- Around line 10-16: selectBrandImage currently can return undefined (leading to
<img src={undefined}>) and won't fail if NetworkBrand gains new values; change
it to use a switch on the brand (function selectBrandImage) and handle every
known NetworkBrand case, and add an exhaustive default that either throws an
Error or returns a safe fallback image; for full type-safety use a never-check
in the default branch (e.g., const _exhaustive: never = brand) so TypeScript
will force you to update selectBrandImage when NetworkBrand changes.
In `@app/src/components/preview/CardPreview.tsx`:
- Line 11: Memoize the networkBrand calculation so
BrandValidator.detectNetworkBrand(cardNumber.join('')).brand only runs when
cardNumber changes: wrap that expression in a useMemo (imported from React) and
use cardNumber.join('') as the dependency (or the cardNumber array if you
prefer) so re-computation occurs only when the card number changes; update the
reference to networkBrand inside the component to use the memoized value.
In `@app/src/hooks/useCardForm.ts`:
- Around line 19-20: The hook useCardForm currently hardcodes brand-specific
digit lengths in the lastDigitLength calculation using
BrandValidator.detectNetworkBrand(...) which reduces reuse and makes maintenance
harder; move this logic into BrandValidator by adding a method like
getExpectedLastDigits(brand) or getExpectedLength(brand) (or export a
BRAND_LENGTHS constant) and replace the inline ternary in useCardForm with a
call to that new API (referencing BrandValidator and lastDigitLength in the
hook) so all brand rules are centralized and reusable across the app.
In `@app/src/validators/CardValidator.ts`:
- Around line 56-65: Refactor isValidCardExpiryDate to remove duplicated
current-year logic by reusing a single computed currentYear/currentMonth
(extract into a helper or use existing isValidYear flow), ensure the function
handles two-digit years deterministically (either convert 2-digit input to full
year using a defined pivot or validate/explicitly document the two-digit-year
assumption), and make sure isValidYear is called before isValidCardExpiryDate so
expiry check only runs after year/month format and range are validated; update
references to isValidCardExpiryDate and isValidYear accordingly so
responsibilities are clear and duplication eliminated.
🪄 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: 0ac73003-b6ce-4297-8d8f-897f12c92870
⛔ Files ignored due to path filters (7)
app/package-lock.jsonis excluded by!**/package-lock.jsonapp/src/assets/Check.svgis excluded by!**/*.svgapp/src/assets/amex-logo.svgis excluded by!**/*.svgapp/src/assets/chevron-down.svgis excluded by!**/*.svgapp/src/assets/chevron-up.svgis excluded by!**/*.svgapp/src/assets/diners-logo.svgis excluded by!**/*.svgapp/src/assets/unionpay-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (40)
README.mdapp/package.jsonapp/src/App.tsxapp/src/components/Card.tsxapp/src/components/form/CardCVCInput.stories.tsxapp/src/components/form/CardCVCInput.tsxapp/src/components/form/CardExpiryDateInput.stories.tsxapp/src/components/form/CardExpiryDateInput.tsxapp/src/components/form/CardForm.stories.tsxapp/src/components/form/CardForm.tsxapp/src/components/form/CardNumberInput.stories.tsxapp/src/components/form/CardNumberInput.tsxapp/src/components/form/CardPasswordInput.stories.tsxapp/src/components/form/CardPasswordInput.tsxapp/src/components/form/CardSection.stories.tsxapp/src/components/form/CardSelectionDropdown.stories.tsxapp/src/components/form/CardSelectionDropdown.tsxapp/src/components/form/ConfirmButton.stories.tsxapp/src/components/form/ConfirmButton.tsxapp/src/components/preview/CardExpiryDate.stories.tsxapp/src/components/preview/CardNetworkBrand.stories.tsxapp/src/components/preview/CardNetworkBrand.tsxapp/src/components/preview/CardNumber.stories.tsxapp/src/components/preview/CardNumber.tsxapp/src/components/preview/CardPreview.stories.tsxapp/src/components/preview/CardPreview.tsxapp/src/constants/cardCompanyOptions.tsapp/src/context/CardContext.tsapp/src/hooks/useCardForm.tsapp/src/hooks/useCardNumberInput.tsapp/src/hooks/useExpiryDateInput.tsapp/src/hooks/useFormFocusChain.tsapp/src/hooks/useSingleInput.tsapp/src/page/Card.tsxapp/src/page/RegistrationComplete.tsxapp/src/style/CardStyles.tsapp/src/types/cardNumber.tsapp/src/types/fieldError.tsapp/src/validators/BrandValidator.tsapp/src/validators/CardValidator.ts
💤 Files with no reviewable changes (2)
- app/src/types/cardNumber.ts
- app/src/components/Card.tsx
Comment on lines
+32
to
+37
| onChange={(e) => { | ||
| onChange(e); | ||
| if (e.target.value.length === 3) { | ||
| onComplete(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
유효하지 않은 3자리 입력에서도 완료 콜백이 실행될 수 있습니다.
현재는 길이만 3이면 onComplete()를 호출해서, 숫자 검증 실패 케이스에서도 다음 단계로 넘어갈 수 있습니다. onComplete 조건을 “길이 3 + 유효성 통과” 기준으로 묶는 방향을 확인해 주세요.
🤖 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 `@app/src/components/form/CardCVCInput.tsx` around lines 32 - 37, The onChange
handler in CardCVCInput currently calls onComplete() solely based on length===3
which permits invalid inputs; modify the handler in CardCVCInput.tsx to call
onComplete() only when the value both has length 3 and passes a CVC validation
(e.g., a helper isValidCVC(value) or a regex like /^\d{3}$/) so non-numeric or
otherwise invalid 3-character inputs do not trigger onComplete; locate the
onChange arrow function and replace the length-only check with a combined
length+validation check (or add and reuse an existing validateCVC function)
before invoking onComplete().
Comment on lines
+14
to
+45
| return ( | ||
| <Wrapper> | ||
| <DropwdownBox | ||
| type="button" | ||
| $isOpen={isDropdownOpen} | ||
| onClick={() => setIsDropdownOpen(!isDropdownOpen)} | ||
| > | ||
| {selectedLabel ? ( | ||
| <span>{selectedLabel}</span> | ||
| ) : ( | ||
| <Placeholder>카드사를 선택해주세요</Placeholder> | ||
| )} | ||
| <img src={isDropdownOpen ? chevronDown : chevronUp} alt="토글 이미지" /> | ||
| </DropwdownBox> | ||
| {isDropdownOpen && ( | ||
| <OptionsBox> | ||
| {cardCompanyOptions.map((option) => ( | ||
| <OptionItem | ||
| key={option.value} | ||
| onClick={() => { | ||
| setCardCompany(option.value); | ||
| setIsDropdownOpen(false); | ||
| onSelect(); | ||
| }} | ||
| > | ||
| {option.label} | ||
| </OptionItem> | ||
| ))} | ||
| </OptionsBox> | ||
| )} | ||
| </Wrapper> | ||
| ); |
There was a problem hiding this comment.
접근성과 사용자 경험 개선을 고려해보세요.
현재 드롭다운 구현에서 몇 가지 개선이 필요한 부분이 있습니다:
- 외부 클릭 처리: 드롭다운 외부를 클릭해도 닫히지 않습니다
- 키보드 내비게이션: 화살표 키, Enter, Escape 키 지원이 없습니다
- ARIA 속성:
aria-expanded,aria-haspopup,role등이 없어 스크린 리더 사용자가 어려움을 겪을 수 있습니다 - 선택된 항목 표시: 옵션 목록에서 현재 선택된 항목을 시각적으로 구분하지 않습니다
생각해볼 점:
useEffect와 document 이벤트 리스너로 외부 클릭을 감지하는 방법은 어떨까요?- WAI-ARIA 드롭다운 패턴 문서를 참고하면 어떨까요?
- HTML
<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 `@app/src/components/form/CardSelectionDropdown.tsx` around lines 14 - 45, Add
accessible behavior and external-click handling: attach a ref to the dropdown
wrapper/DropwdownBox and in a useEffect add/remove a document
mousedown/touchstart listener to close the menu when clicking outside (use
isDropdownOpen and setIsDropdownOpen). Add keyboard handling on DropwdownBox and
option elements to support ArrowUp/ArrowDown to move a focused option index,
Enter to select (calling setCardCompany and onSelect), and Escape to close;
manage focus with refs or a focusedIndex state. Add WAI-ARIA attributes: on
DropwdownBox set role="combobox" (or role="button") with aria-haspopup="listbox"
and aria-expanded={isDropdownOpen}; on OptionsBox set role="listbox"; on each
OptionItem set role="option" and aria-selected={option.value === currentValue}.
Finally visually indicate the current selection by applying a selected
style/class to the OptionItem when option.value === the selected cardCompany.
Comment on lines
+21
to
+30
| const isFormComplete = | ||
| cardCompany !== '' && | ||
| cardNumber[0].length === 4 && | ||
| cardNumber[1].length === 4 && | ||
| cardNumber[2].length === 4 && | ||
| cardNumber[3].length === lastDigitLength && | ||
| cardExpiryDate['expiry-month'].length === 2 && | ||
| cardExpiryDate['expiry-year'].length === 2 && | ||
| cardCVC.length === 3 && | ||
| cardPassword.length === 2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
isFormComplete가 유효성까지 검사해야 할까요?
현재 isFormComplete는 각 필드의 길이만 확인합니다. 예를 들어:
- 만료일이 "00/00"이어도 통과
- 이미 지난 날짜(예: "01/20")여도 통과
- CVC가 "000"이어도 통과
CardValidator.ts에 유효성 검증 로직이 있는데, isFormComplete가 이를 활용하지 않고 있습니다.
생각해볼 점:
- "완료"와 "유효함"의 차이는 무엇일까요?
- 제출 버튼 활성화 조건으로 길이만 체크하는 것이 적절할까요?
- 실시간 유효성 검사 결과를
isFormComplete계산에 포함시키면 어떨까요?
폼의 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 `@app/src/hooks/useCardForm.ts` around lines 21 - 30, Replace the current
length-only check in useCardForm's isFormComplete (which inspects cardCompany,
cardNumber[], lastDigitLength, cardExpiryDate['expiry-month'|'expiry-year'],
cardCVC, cardPassword) with actual validation calls to the CardValidator API:
call the card number validator for the assembled number, the expiry validator
for month/year, the CVC validator for cardCVC, and the password validator for
cardPassword (or equivalent isValid* helpers exported from CardValidator.ts) and
combine their boolean results so isFormComplete reflects real validity rather
than just string lengths.
|
|
||
| const result = onChangeValidator(value); | ||
| if (!result.valid) { | ||
| console.log(value); |
There was a problem hiding this comment.
민감 입력값 콘솔 출력은 제거가 필요합니다.
useSingleInput이 카드 CVC/비밀번호에도 쓰이기 때문에, Line 19 로그는 민감 데이터 노출 리스크가 있습니다. 로그를 제거하는 쪽이 안전합니다.
🤖 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 `@app/src/hooks/useSingleInput.ts` at line 19, Remove the sensitive console
output from the useSingleInput hook by deleting the console.log(value) call (the
debug print inside useSingleInput) so CVC/password inputs are not logged; if
debugging is needed, replace with a non-production-safe mechanism such as gated
logging behind an explicit dev-only flag or use secure dev-only breakpoints, but
do not leave any console output of the input value in the hook.
Comment on lines
+9
to
+17
| const { firstDigits, cardCompany } = location.state || {}; | ||
| const cardCompanyLabel = cardCompanyOptions.find((option) => option.value === cardCompany)?.label; | ||
|
|
||
| return ( | ||
| <Container> | ||
| <img src={Check} alt={'체크 사진'}></img> | ||
| <Heading> | ||
| <span>{firstDigits}로 시작하는</span> | ||
| <span>{cardCompanyLabel}가 등록되었어요.</span> |
There was a problem hiding this comment.
라우트 상태가 없을 때의 처리를 고려해보세요.
사용자가 URL을 직접 입력하거나 새로고침하면 location.state가 null이 되어 firstDigits와 cardCompanyLabel이 undefined가 됩니다. 이 경우 "undefined로 시작하는 undefined가 등록되었어요"와 같은 텍스트가 표시될 수 있습니다.
이런 상황을 어떻게 처리하면 좋을까요? 몇 가지 접근 방법이 있습니다:
- 상태가 없을 때 리다이렉션하기
- 기본값 표시 또는 에러 메시지 보여주기
- 조건부 렌더링으로 완료 화면 숨기기
어떤 방식이 사용자 경험에 가장 적합할지 고민해보세요.
🤖 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 `@app/src/page/RegistrationComplete.tsx` around lines 9 - 17, The component
reads firstDigits and cardCompany from location.state and computes
cardCompanyLabel, but doesn't handle missing state; update the
RegistrationComplete component to guard against null/undefined location.state
(or missing firstDigits/cardCompany): choose one approach—redirect to the prior
page when state is absent, or render a fallback UI with default
values/message—and implement it by checking location.state (and the derived
firstDigits/cardCompanyLabel) before rendering; use the existing symbols
(location.state, firstDigits, cardCompany, cardCompanyLabel, cardCompanyOptions,
and the RegistrationComplete component) to locate the logic and add either a
conditional early return (redirect) or conditional rendering with a clear
fallback message.
Comment on lines
+42
to
+43
| ## 기능 요구 사항 | ||
|
|
There was a problem hiding this comment.
중복 헤딩으로 문서 린트 경고가 발생합니다.
Line 42의 ## 기능 요구 사항이 앞선 섹션과 동일해서 MD024 경고가 납니다. Step2 섹션 제목을 유니크하게 바꿔 주세요.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 42-42: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 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 `@README.md` around lines 42 - 43, 문서에서 중복된 섹션 제목 "## 기능 요구 사항"으로 인해 MD024 경고가
발생하므로 Step2 섹션의 헤더 텍스트를 고유하게 변경하세요; 파일 내 기존 헤더 "## 기능 요구 사항"과 충돌하는 두 번째 헤더(현재
"## 기능 요구 사항"으로 표기된 Step2)를 찾아 이름을 예를 들어 "## 기능 요구 사항 (Step2)" 또는 더 구체적인 "## 기능
요구 사항 — Step2: 세부사항"처럼 유니크한 텍스트로 바꾸고, 관련된 내부 링크나 참조가 있다면 동일한 새로운 헤더 텍스트로 함께
갱신하세요.
…view and useCardNumberInput
jw-r
requested changes
May 13, 2026
jw-r
left a comment
jw-r
left a comment
There was a problem hiding this comment.
벤지께서 고민하셨던 과정을 직접 들어서 굉장히 재미있었습니다🤩
너무 무리하지 않는 선에서 시도해주세요👍
하나하나 전부 설명이 가능해야합니다!
Member
Author
|
개발자도구에서 disabled 속성을 제거하거나 display none을 해제하여 폼이 완료되지 않은 상태에서도 navigate가 발생하는 현상을 방지하기 위해, 또한 input 상태들과 onComplete 함수들을 하나의 객체로 묶는 방식도 고려하였으나, 객체 내 하나의 상태가 변경될 때 해당 객체를 참조하는 컴포넌트에서 불필요한 리렌더링이 발생할 수 있고 코드 가독성 측면에서도 직관적이지 않다고 보아 각각 분리된 상태로 유지하였습니다.
에러 상태 관리와 관련하여, 각 input의 검증 로직과 에러 state를 마지막으로 위의 PR Description에서의 4가지 질문 관련해서도 궁금한점이 남아있습니다ㅠ 감사합니다! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 페이먼츠
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
2단계
3단계
idle | loading | success | error네 가지로 명시적으로 관리한다.🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 우디!
스텝2도 잘 부탁드립니다:) 😇
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
스텝2에서 가장 많이 고민한 부분은 카드 번호 상태 구조와 커스텀 훅을 어떤 기준으로 나눌 것인가 였습니다.
카드 번호를
string하나로 관리할지,string[]배열로 관리할지 고민했는데, 각 Input이 4자리 단위로 독립적인 validation을 받아야 하고 UI도 4개의 Input을 렌더링하는 구조이기 때문에 배열이 더 자연스럽다고 판단했습니다. 합산이 필요한 경우엔 cardNumber.join('')으로 충분했고, 이 과정에서 파생 상태는 별도 useState로 관리하지 않고 const로 계산하면 된다는 것을 체감했습니다. networkBrand가 대표적인 예로, 카드 번호 배열로부터 매 렌더링마다 계산되는 값이기 때문에 state가 아닌 변수로 처리했습니다.이 과정에서 파생상ㅇ태는
useState가 아닌const로 계산하면 된다는 것을 체감했습니다!네트워크 브랜드 감지 로직은 처음에 Visa, Mastercard만 허용하고 나머지를 에러 처리하는 방식이었는데, 카드 번호는 숫자 패턴으로 브랜드를 판별할 수 있기 때문에 정규식 기반으로 리팩터링했습니다. 로직이 커지면서
BrandValidator를 별도 파일로 분리했고, 브랜드에 따라 마지막 Input의 maxLength와 placeholder를 동적으로 바꾸는 handleInputMaxLength도 함께 구현했습니다.커스텀 훅은 재사용 가능한가와 관심사 분리가 필요한가 두 가지 기준으로 판단했습니다. CardCVCInput과 CardPasswordInput은 onChange 검증 -> 에러 상태 관리 -> onBlur 검증이라는 동일한 패턴을 공유하기 때문에
useSingleInput으로 추상화해 재사용했습니다. 반면CardNumberInput과CardExpiryDateInput은 각자 고유한 로직(브랜드 감지, 월/연도 분리 검증 등)이 있어서 무리하게 추상화하면 오히려 복잡해진다고 판단해 컴포넌트 전용 훅으로 분리했습니다. 저는 '재사용이 가능한가'에 대한 집착이 있었습니다. 계속해서 중복을 제거하고 어떻게 하면 재사용 가능한 커스텀 훅으로 추출시킬 수 있을까에 대한 고민이 많았는데 결국 컴포넌트 전용 훅으로 분리하는 과정을 거치며 추상화는 단순히 중복 제거가 아니라 복잡도를 낮출 수 있을 때 의미 있다는 것을 깨달았습니다!포커스 체인은 각 훅 내부의 ref는 해당 훅에서 관리하되, 폼 전체의 섹션 간 포커스 이동은 useFormFocusChain으로 모아서 CardForm에서 한 곳에서 관리하도록 했습니다. ref가 어떻게 DOM과 연결되는지 직접 추적하면서 setState가 비동기로 동작해 현재 렌더링이 끝난 뒤에야 새 값이 반영된다는 것도 이번 단계에서 명확하게 이해하게 됐습니다.
2) 이번 리뷰를 통해 논의하고 싶은 부분
1. 커스텀 훅의 추상화 수준
이번 단계에서 컴포넌트는 UI 렌더링만 담당하고, 비즈니스 로직은 커스텀 훅이 가져가는 구조가 정말 마음에 들었습니다. 다만 현재 커스텀 훅 하나가
onChange,onBlur,fieldErrors등 여러 역할을 함께 가지고 있는데, 이를 더 잘게 쪼개는 것이 맞는지 고민해봤습니다. 하지만 이렇게 했을 때 오히려 과한 추상화가 될 것 같았고 현재 크기가 비대하다고 느껴지지도 않아서 지금 구조를 유지하기로 결정했는데, 현재 추상화 수준이 적절한지 우디의 의견이 궁금합니다!2. 포커스 관련 훅의 통합 여부
useFormFocusChain에서 폼 전체의 섹션 간 포커스 이동을 위한ref와 콜백 그리고 단계별 섹션 노출을 위한currentStep을 함께 관리하고, 각 컴포넌트 전용 훅(useCardNumberInput,useExpiryDateInput) 내부에서도 필드 간 자동 포커스를 위한ref를 따로 관리하고 있습니다. 포커스 관련 로직을 하나의 훅으로 통합하는 것도 고려해봤는데, 그렇게 되면 훅 간 종속성이 복잡해질 것 같았습니다. 각 커스텀 훅이 독립적으로 동작하는 것이 더 좋지 않을까 판단해 현재 구조를 유지했는데, 포커스 관련 로직을 통합하는 것과 분산시키는 것 중 어떤 방식이 더 효율적인지 우디의 의견이 궁금합니다!3. 상태 흐름의 일관성
모든 카드 상태가 CardContext 하나에서 관리되고, 데이터 흐름이 Context -> 훅 -> setter 호출로 단방향을 유지하고 있어서 충분히 직관적이고 일관된 구조라고 생각했습니다. 다만 현재 Context에 상태들의 setter들을 직접 노출하고 있는데, 규모가 커졌을 때 의도치 않은 상태 변경이 생길 수 있는 구조이기도 한 것 같습니다 (아무래도 어떤 컴포넌트던지 setter에 접근할 수 있는 방식이다보니). 지금 규모에서 이 방식이 적절한지, 아니면 setter 대신 의도가 명확한 핸들러 함수(setter를 포함하는)를 정의하여 settter 대신 노출하는 방식으로 개선하는 게 나을지 우디의 의견이 궁금합니다!
4. isFormComplete 판단 위치
현재 isFormComplete는 useCardForm 안에서 각 필드의 길이 조건을 직접 계산해 CardContext를 통해 내려주고 있습니다. 처음엔 CardForm 컴포넌트 안에서 직접 계산하고 있었는데, 컴포넌트가 필드별 유효 길이까지 알고 있는 건 UI 역할을 넘어선다고 판단해 훅으로 옮겼습니다. 하지만 더 깊게 생각해봤을 때 버튼 활성화 조건처럼 UI와 밀접한 로직은 컴포넌트 가까이 두는 것도 괜찮지 않을까?라는 생각이 들었습니다.
이 상황에서 UI와 밀접한 로직은 컴포넌트에 가까이 두는 것이 괜찮을지 혹은 이러한 로직을 도메인 규칙으로 보고 훅에서 관리하는 게 맞는지 우디의 의견이 궁금합니다!
스텝2에서의 컴포넌트 설계, 데이터 플로우, 그리고 커스텀 훅의 역할에 대해 다이어그램으로 정리해보았습니다!
컴포넌트 설계도
graph TD App --> Card App --> RegistrationComplete Card --> CardContext["CardContext.Provider"] CardContext --> CardContainer CardContainer --> CardPreview CardContainer --> CardForm CardPreview --> CardNetworkBrand CardPreview --> CardNumber CardPreview --> CardExpiryDate CardForm --> CardSection1["CardSection (카드번호)"] CardForm --> CardSection2["CardSection (카드사)"] CardForm --> CardSection3["CardSection (유효기간)"] CardForm --> CardSection4["CardSection (CVC)"] CardForm --> CardSection5["CardSection (비밀번호)"] CardForm --> ConfirmButton CardSection1 --> CardNumberInput CardSection2 --> CardSelectionDropdown CardSection3 --> CardExpiryDateInput CardSection4 --> CardCVCInput CardSection5 --> CardPasswordInput데이터 플로우
flowchart TD subgraph Card["Card.tsx (Page)"] useCardForm["useCardForm()\n카드 상태 초기화"] end subgraph Context["CardContext"] state["cardNumber\ncardExpiryDate\ncardCompany\ncardCVC\ncardPassword"] setters["setCardNumber\nsetCardExpiryDate\nsetCardCompany\nsetCardCVC\nsetCardPassword"] end subgraph Hooks["Custom Hooks (form components 내부)"] useCardNumberInput useExpiryDateInput useSingleInput["useSingleInput\n(CVC, Password 공용)"] end subgraph Validators CardValidator BrandValidator end useCardForm -- "상태·setter 제공" --> Context Context -- "useCardContext()" --> Hooks Hooks -- "validator 호출" --> Validators Validators -- "ValidationResult" --> Hooks Hooks -- "setter 호출" --> Context Context -- "useCardContext()" --> CardPreview Context -- "useCardContext()" --> CardForm커스텀 훅 역할도
flowchart LR subgraph Hooks useCardForm["useCardForm\n카드 전체 상태 초기화\n(5개 useState)"] useCardContext["useCardContext\nContext 안전 접근\n(throw if no provider)"] useCardNumberInput["useCardNumberInput\n카드번호 4분할 입력\n네트워크 브랜드 감지\n자동 포커스 이동\n필드별 에러 관리"] useExpiryDateInput["useExpiryDateInput\nMM/YY 입력\n월·연도 유효성 검사\n만료일 검사\n자동 포커스 이동"] useSingleInput["useSingleInput\n단일 입력 범용 훅\nonChange · onBlur 처리\n에러 상태 관리"] useFormFocusChain["useFormFocusChain\n필드 간 ref 관리\n포커스 체인 콜백 제공\n단계별 섹션 노출 (currentStep)"] end Card.tsx --> useCardForm CardForm.tsx --> useCardContext CardForm.tsx --> useFormFocusChain CardNumberInput.tsx --> useCardNumberInput CardExpiryDateInput.tsx --> useExpiryDateInput CardCVCInput.tsx --> useSingleInput CardPasswordInput.tsx --> useSingleInput useCardNumberInput --> useCardContext useExpiryDateInput --> useCardContext✅ 리뷰어 체크 포인트
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*를 사용했는가?