[페이먼츠 2단계 - hooks & state] 코브(윤태훈) 미션 제출합니다. #533
Conversation
|
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은 카드 결제 애플리케이션을 단계별 필드 렌더링과 라우팅으로 구현합니다. 카드 네트워크별 동적 세그먼트 감지, 단계별 필드 조건부 표시, 완료 페이지 라우팅을 추가하고, 기존 Section 컴포넌트를 Field 기반 아키텍처로 리팩토링합니다. 예상 코드 리뷰 시간🎯 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: 10
🧹 Nitpick comments (7)
src/components/cardInfo/CardInfo.styles.ts (1)
41-46: ⚡ Quick win
Title을h1로 고정하면 헤딩 계층이 깨질 수 있습니다.이 컴포넌트가 여러 필드에서 반복 사용되면 한 화면에
h1이 여러 개 생길 수 있습니다. 실제 문서 헤딩과 스타일용 타이포그래피를 분리하는 구조를 고려해 주세요.🤖 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 `@src/components/cardInfo/CardInfo.styles.ts` around lines 41 - 46, The styled component Title is hard-coded to an h1 which can create multiple h1s and break heading semantics; change Title to render a non-semantic element (e.g., span/div) or make it polymorphic so callers can pass the semantic tag (use the "as" prop or a separate Heading component), keep the same styles (font-size, font-weight, line-height, color) and update usages to provide the appropriate heading level where semantic structure matters; locate the Title export in CardInfo.styles.ts and adjust its base element and/or API accordingly.src/constants/constants.ts (1)
35-44: ⚡ Quick win
CARD_BRANDS타입을 키-유니온 기반으로 좁히는 게 안전합니다.현재
Record<string, ...>는 임의 문자열 키를 모두 허용해서, 오타나 미등록 브랜드 접근을 컴파일 타임에 잡기 어렵습니다.keyof기반으로 키를 제한하면 후속 컴포넌트 인덱싱 안정성이 크게 좋아집니다.🤖 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 `@src/constants/constants.ts` around lines 35 - 44, Replace the overly-broad Record<string,...> for CARD_BRANDS with a key-union constrained type: make the literal object a const (so keys are preserved) and derive a CardBrand union via keyof typeof CARD_BRANDS, then use that union to type the map (e.g., Record<CardBrand, { label: string; color: string }> or let the const object infer the value types). Update any usages that index CARD_BRANDS to use the new CardBrand type for safer compile-time checking; refer to the CARD_BRANDS symbol to locate the declaration and consumer sites.src/utils/cardNetwork.ts (1)
39-57: ⚖️ Poor tradeoff카드 네트워크가 변경될 때 사용자 입력이 유실되는 동작이 의도된 것인가요?
Lines 50-54에서 세그먼트 개수가 변경될 때 첫 번째 세그먼트만 보존하고 나머지는 빈 문자열로 채우고 있습니다.
예를 들어 사용자가
["1234", "5678"]을 입력했는데 네트워크 감지 결과가 3개 세그먼트로 변경되면["1234", "", ""]또는[matchedPrefix, "", ""]가 됩니다. 이 경우 "5678"이 사라지게 되는데요.질문:
- 이러한 데이터 유실이 의도된 UX인가요?
- 사용자가 입력 중일 때 이런 상황이 자주 발생할까요?
- 입력된 전체 숫자를 보존하면서 새로운 세그먼트 구조로 재분배하는 방법을 고려해보셨나요?
힌트:
joined변수에는 전체 숫자가 있으므로, 이를newSegments길이에 맞게 재분배하는 로직을 생각해볼 수 있습니다.🤖 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 `@src/utils/cardNetwork.ts` around lines 39 - 57, The current reshapeCardNumber drops user-entered digits when card network segment count changes; update reshapeCardNumber to preserve the full concatenated input (joined) and redistribute it into the newSegments structure instead of blanking most segments. Specifically, when newSegments.length !== updated.length, compute segment target lengths from newSegments (e.g., newSegments.map(s => s.length) or from the network pattern), then slice joined into those lengths to populate a result array of that length (filling trailing segments with '' if joined is shorter), and return that array; still use detectCardNetworkMatch(match) only to choose an appropriate prefix if you need a fallback for the first segment. This keeps getCardNumberSegments, detectCardNetworkMatch and the joined variable but changes the branch to re-split joined into the new segment shape rather than discarding digits.src/components/cardInfo/cardNumber/CardNumberField.tsx (1)
22-22: ⚡ Quick win세그먼트 계산 최적화를 고려해보세요.
22번 줄의
getCardNumberSegments가 매 렌더마다 실행됩니다.cardNumber가 변경될 때만 재계산하도록useMemo를 사용하면 불필요한 연산을 줄일 수 있습니다.const segments = useMemo( () => getCardNumberSegments(cardNumber.join('')), [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 `@src/components/cardInfo/cardNumber/CardNumberField.tsx` at line 22, The call to getCardNumberSegments(cardNumber.join('')) in CardNumberField is executed on every render; wrap this computation in React's useMemo so it only recalculates when cardNumber changes (e.g., compute segments via useMemo(() => getCardNumberSegments(cardNumber.join('')), [cardNumber])), referencing the symbols getCardNumberSegments, cardNumber and segments to replace the direct call with a memoized value.src/components/cardInfo/isComplete.ts (1)
9-17: 💤 Low value카드 번호 세그먼트 처리에 대해 생각해볼 점이 있습니다.
10번째 줄에서
cardNumber.join('')으로 문자열을 합친 후 다시getCardNumberSegments를 호출해 세그먼트 길이를 가져오고 있습니다.이미
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 `@src/components/cardInfo/isComplete.ts` around lines 9 - 17, The isCardNumberComplete function currently recomputes segment lengths by calling getCardNumberSegments(cardNumber.join('')), which is redundant because cardNumber is already an array of segments; instead, avoid joining and recomputing—use the provided segments directly (e.g., derive expected segment lengths from cardNumber.length or accept segment lengths as an argument) and only call getCardNumberSegments when you genuinely need to infer segments from the raw concatenated number. Update isCardNumberComplete to compare cardNumber.length to the expected segment count without joining, keep the existing validations (validateNumber and validateCardNumberLength), and adjust callers or getCardNumberSegments signature if you need to obtain canonical segment shapes elsewhere; reference isCardNumberComplete, getCardNumberSegments, validateNumber, and validateCardNumberLength when making the change.src/components/cardInfo/CardInfo.tsx (1)
17-21: 💤 Low value필드 렌더링 순서가 JSX에서 역순입니다.
CardInfo.tsx의 17-21줄에서 필드가 역순으로 렌더링되고 있습니다 (password → cvc → expiry → brand → number). CardInfo.styles.ts의 Wrapper는
flex-direction: column을 사용하므로 CSS에서 순서를 뒤집지 않습니다.JSX 코드 자체에서 역순 렌더링이 발생하고 있으며, 이는 step 단계와 관계없이 의도적인 것으로 보입니다. 그러나 유지보수성을 위해:
- 의도가 명확하지 않다면 일반적인 순서(number → brand → expiry → cvc → password)로 정렬하거나
- 현재 순서가 필요한 경우 주석을 추가하여 의도를 문서화하는 것을 권장합니다.
🤖 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 `@src/components/cardInfo/CardInfo.tsx` around lines 17 - 21, The JSX renders card fields in reverse order (CardPasswordField → CvcField → ExpireDateField → CardBrandField → CardNumberField) which conflicts with the visual column flow; reorder the render calls in CardInfo.tsx so they follow the natural sequence CardNumberField, CardBrandField, ExpireDateField, CvcField, CardPasswordField (using the existing cardForm.{cardNumber, cardBrand, expireDate, cvc, cardPassword} and step gating), or if the reverse order is intentional, add a clear comment above the block explaining the rationale and why CSS ordering isn’t used (reference the components CardNumberField, CardBrandField, ExpireDateField, CvcField, CardPasswordField and the step variable).src/components/cardPreview/CardPreview.tsx (1)
23-24: ⚡ Quick win브랜드 표현의 단일 소스 규칙을 한 번 정리해보면 좋겠습니다.
현재 로고는 카드번호 기반(Line 23), 배경색은 선택 브랜드 기반(Line 24)이라 서로 다른 상태를 보여줄 수 있습니다.
우선순위(예: “선택값 우선” vs “번호 감지 우선”)를 한 곳에서 결정하도록 맞추면 UI 일관성이 좋아집니다.🤖 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 `@src/components/cardPreview/CardPreview.tsx` around lines 23 - 24, The card logo and background are derived from different sources (cardBrandLogoSrc uses getCardBrandLogoSrc(cardNumber.join('')) while backgroundColor uses cardBrand/CARD_BRANDS), causing inconsistent UI; pick a single priority rule (e.g., prefer explicit cardBrand if present, otherwise detect from cardNumber) and implement it by computing a resolvedBrand variable (resolve from cardBrand || detect via getCardBrandLogoSrc/getCardBrandFromNumber) and then use that resolvedBrand to set both cardBrandLogoSrc and backgroundColor (handling undefined/null when neither yields a brand).
🤖 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 `@README.md`:
- Line 139: Update the Markdown emphasis so there is no space before the closing
emphasis markers: change the fragment containing "**\*로 시작하는 **" in README.md to
"**\*로 시작하는**" (remove the space before the closing **) to resolve the MD037
warning about emphasis markers.
In `@src/components/cardInfo/cardBrand/CardBrandField.tsx`:
- Around line 25-37: The dropdown lacks ARIA relationships and focusable
options; replace the empty Label with a meaningful label text (or give it an id
and use aria-labelledby) and make Trigger a real button or element with
aria-haspopup="listbox" and aria-expanded={isOpen}; give the DropdownList an id
and role="listbox" and aria-labelledby pointing to the label/trigger; change
DropdownItem to be focusable options (either <button> or elements with
role="option" and tabIndex=0) and wire keyboard handlers on DropdownItem to call
handleSelect(brand) on Enter/Space and support ArrowUp/ArrowDown to move focus;
ensure setIsOpen(false) runs on selection and manage focus return to Trigger;
reference Label, Trigger, DropdownList, DropdownItem, handleSelect, setIsOpen,
CARD_BRANDS, cardBrand, and isOpen when applying these changes.
In `@src/components/cardInfo/cardNumber/useCardNumberValidation.ts`:
- Around line 8-21: The validate function in useCardNumberValidation iterates
segments and early-returns false on segment validation failure (validateNumber)
but always returns true after running validateCardNumberLength and setting
setError; change this so the function returns the actual lengthResult.state
(i.e., if validateCardNumberLength fails, setError(lengthResult.message) and
return false), keeping the existing early-return behavior for validateNumber;
locate validate, validateNumber, validateCardNumberLength and setError to
implement this consistent return logic (matching the pattern used in
useCardPasswordValidation).
In `@src/components/cardInfo/cardPassword/CardPasswordField.tsx`:
- Around line 22-26: The validate function in useCardPasswordValidation is
incorrectly returning true regardless of length check results; update the
function (the validate implementation that calls validateCardPasswordLength) to
return the actual length check state (i.e., return lengthResult.state) instead
of always true so that CardPasswordField.handleChange's guard (if
(!validate(newValue)) return;) works and setPassword is only called for valid
inputs.
In `@src/components/cardInfo/cardPassword/useCardPasswordValidation.ts`:
- Around line 7-18: The validate function incorrectly always returns true after
running validateCardPasswordLength; change its return logic to mirror the
numeric check: call validateCardPasswordLength(newValue), update setError to
lengthResult.state ? "" : lengthResult.message, and return lengthResult.state
(i.e., false when length validation fails, true when it passes) so callers like
CardPasswordField's usage (which only applies updates when validate is true)
behave correctly; keep validateNumber and its existing early-return behavior
unchanged.
In `@src/components/cardInfo/cvc/useCvcValidation.ts`:
- Around line 7-18: The validate function in useCvcValidation returns true
regardless of the length check result causing inconsistent behavior (it returns
false on number validation failure but always true after length validation);
change the final return from always-true to return lengthResult.state so the
function reflects the length validation outcome, and then audit the same
validate implementations in useCardPasswordValidation and
useCardNumberValidation to make the same change (or explicitly decide to allow
value updates on length failures and adjust caller UX accordingly).
In `@src/components/cardInfo/expireDate/useExpireDateValidation.ts`:
- Around line 16-25: The function currently returns true even when validation
fails; change the logic so validateMonth(updatedExpireDate[0]) failure sets
setError(monthResult.message) and returns false (not true), and after calling
expireDateResult = validateExpireDateNotPast(updatedExpireDate) setError to ""
on success or expireDateResult.message on failure and return
expireDateResult.state (i.e., return false on failure, true on success) instead
of always returning true; update the branches around validateMonth, setError,
and the final return to use validateMonth, validateExpireDateNotPast, setError,
and updatedExpireDate as the referenced symbols.
In `@src/components/cardInfo/useCardStep.ts`:
- Around line 21-26: The inline calls to setStep() must be moved into a
useEffect so state updates occur in an effect instead of during render: create a
useEffect that depends on the cardForm fields used (cardForm.cardNumber.value,
cardForm.cardBrand.value, cardForm.expireDate.value, cardForm.cvc.value) and
isAllComplete, compute the correct targetStep by running the same guards
(isCardNumberComplete, isCardBrandComplete, isExpireDateComplete, isCvcComplete,
isAllComplete) and call setStep(targetStep) only if targetStep !== step to avoid
redundant updates; update references to setStep, cardForm and the predicate
functions in that effect and remove the direct setStep(...) calls from the
component body.
In `@src/components/completePage/CompletePage.tsx`:
- Around line 14-18: The route state check only guards against null but not
malformed shapes, so ensure the state conforms to the expected LocationState
shape by verifying that state.cardNumberFirstSegment and state.cardBrand are
typeof "string" before using them; if either check fails, return the same
fallback (<Navigate to="/" replace />). Update the logic around location.state
(and the LocationState type usage) to perform these string-type checks for
cardNumberFirstSegment and cardBrand and redirect when they are missing or not
strings.
In `@src/utils/cardNetwork.ts`:
- Around line 8-28: detectCardNetworkMatch can falsely match empty strings via
the ranges branch because Number("") === 0; add an early guard at the start of
detectCardNetworkMatch to return null for cardNumber === "" (and consider also
trimming and rejecting non-digit input) so range checks never run on empty
input, update the function that iterates CARD_NETWORKS (detectCardNetworkMatch)
to check cardNumber.length === 0 before prefixes/ranges, and add unit tests for
edge cases (empty string, 1-2 digit numbers, non-digit characters) to prevent
regressions.
---
Nitpick comments:
In `@src/components/cardInfo/CardInfo.styles.ts`:
- Around line 41-46: The styled component Title is hard-coded to an h1 which can
create multiple h1s and break heading semantics; change Title to render a
non-semantic element (e.g., span/div) or make it polymorphic so callers can pass
the semantic tag (use the "as" prop or a separate Heading component), keep the
same styles (font-size, font-weight, line-height, color) and update usages to
provide the appropriate heading level where semantic structure matters; locate
the Title export in CardInfo.styles.ts and adjust its base element and/or API
accordingly.
In `@src/components/cardInfo/CardInfo.tsx`:
- Around line 17-21: The JSX renders card fields in reverse order
(CardPasswordField → CvcField → ExpireDateField → CardBrandField →
CardNumberField) which conflicts with the visual column flow; reorder the render
calls in CardInfo.tsx so they follow the natural sequence CardNumberField,
CardBrandField, ExpireDateField, CvcField, CardPasswordField (using the existing
cardForm.{cardNumber, cardBrand, expireDate, cvc, cardPassword} and step
gating), or if the reverse order is intentional, add a clear comment above the
block explaining the rationale and why CSS ordering isn’t used (reference the
components CardNumberField, CardBrandField, ExpireDateField, CvcField,
CardPasswordField and the step variable).
In `@src/components/cardInfo/cardNumber/CardNumberField.tsx`:
- Line 22: The call to getCardNumberSegments(cardNumber.join('')) in
CardNumberField is executed on every render; wrap this computation in React's
useMemo so it only recalculates when cardNumber changes (e.g., compute segments
via useMemo(() => getCardNumberSegments(cardNumber.join('')), [cardNumber])),
referencing the symbols getCardNumberSegments, cardNumber and segments to
replace the direct call with a memoized value.
In `@src/components/cardInfo/isComplete.ts`:
- Around line 9-17: The isCardNumberComplete function currently recomputes
segment lengths by calling getCardNumberSegments(cardNumber.join('')), which is
redundant because cardNumber is already an array of segments; instead, avoid
joining and recomputing—use the provided segments directly (e.g., derive
expected segment lengths from cardNumber.length or accept segment lengths as an
argument) and only call getCardNumberSegments when you genuinely need to infer
segments from the raw concatenated number. Update isCardNumberComplete to
compare cardNumber.length to the expected segment count without joining, keep
the existing validations (validateNumber and validateCardNumberLength), and
adjust callers or getCardNumberSegments signature if you need to obtain
canonical segment shapes elsewhere; reference isCardNumberComplete,
getCardNumberSegments, validateNumber, and validateCardNumberLength when making
the change.
In `@src/components/cardPreview/CardPreview.tsx`:
- Around line 23-24: The card logo and background are derived from different
sources (cardBrandLogoSrc uses getCardBrandLogoSrc(cardNumber.join('')) while
backgroundColor uses cardBrand/CARD_BRANDS), causing inconsistent UI; pick a
single priority rule (e.g., prefer explicit cardBrand if present, otherwise
detect from cardNumber) and implement it by computing a resolvedBrand variable
(resolve from cardBrand || detect via
getCardBrandLogoSrc/getCardBrandFromNumber) and then use that resolvedBrand to
set both cardBrandLogoSrc and backgroundColor (handling undefined/null when
neither yields a brand).
In `@src/constants/constants.ts`:
- Around line 35-44: Replace the overly-broad Record<string,...> for CARD_BRANDS
with a key-union constrained type: make the literal object a const (so keys are
preserved) and derive a CardBrand union via keyof typeof CARD_BRANDS, then use
that union to type the map (e.g., Record<CardBrand, { label: string; color:
string }> or let the const object infer the value types). Update any usages that
index CARD_BRANDS to use the new CardBrand type for safer compile-time checking;
refer to the CARD_BRANDS symbol to locate the declaration and consumer sites.
In `@src/utils/cardNetwork.ts`:
- Around line 39-57: The current reshapeCardNumber drops user-entered digits
when card network segment count changes; update reshapeCardNumber to preserve
the full concatenated input (joined) and redistribute it into the newSegments
structure instead of blanking most segments. Specifically, when
newSegments.length !== updated.length, compute segment target lengths from
newSegments (e.g., newSegments.map(s => s.length) or from the network pattern),
then slice joined into those lengths to populate a result array of that length
(filling trailing segments with '' if joined is shorter), and return that array;
still use detectCardNetworkMatch(match) only to choose an appropriate prefix if
you need a fallback for the first segment. This keeps getCardNumberSegments,
detectCardNetworkMatch and the joined variable but changes the branch to
re-split joined into the new segment shape rather than discarding digits.
🪄 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: 89a277fa-f7dc-4d85-ab2f-4236cfb9e763
⛔ Files ignored due to path filters (5)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/assets/American_Express.svgis excluded by!**/*.svgsrc/assets/China_UnionPay.svgis excluded by!**/*.svgsrc/assets/Diners_Club.svgis excluded by!**/*.svgsrc/assets/check.svgis excluded by!**/*.svg
📒 Files selected for processing (53)
.github/workflows/deploy.ymlREADME.mdpackage.jsonsrc/App.tsxsrc/components/PaymentWidget.styles.tssrc/components/PaymentWidget.tsxsrc/components/cardInfo/CardInfo.stories.tsxsrc/components/cardInfo/CardInfo.styles.tssrc/components/cardInfo/CardInfo.tsxsrc/components/cardInfo/cardBrand/CardBrandField.stories.tsxsrc/components/cardInfo/cardBrand/CardBrandField.styles.tssrc/components/cardInfo/cardBrand/CardBrandField.tsxsrc/components/cardInfo/cardInfoHeader/CardInfoHeader.stories.tsxsrc/components/cardInfo/cardInfoHeader/CardInfoHeader.styles.tssrc/components/cardInfo/cardInfoHeader/CardInfoHeader.tsxsrc/components/cardInfo/cardNumber/CardNumberField.stories.tsxsrc/components/cardInfo/cardNumber/CardNumberField.tsxsrc/components/cardInfo/cardNumber/CardNumberSection.stories.tsxsrc/components/cardInfo/cardNumber/CardNumberSection.tsxsrc/components/cardInfo/cardNumber/useCardNumber.tssrc/components/cardInfo/cardNumber/useCardNumberValidation.tssrc/components/cardInfo/cardPassword/CardPasswordField.stories.tsxsrc/components/cardInfo/cardPassword/CardPasswordField.tsxsrc/components/cardInfo/cardPassword/useCardPasswordValidation.tssrc/components/cardInfo/cvc/CvcField.stories.tsxsrc/components/cardInfo/cvc/CvcField.tsxsrc/components/cardInfo/cvc/CvcSection.stories.tsxsrc/components/cardInfo/cvc/CvcSection.tsxsrc/components/cardInfo/cvc/useCvcValidation.tssrc/components/cardInfo/expireDate/ExpireDateField.stories.tsxsrc/components/cardInfo/expireDate/ExpireDateField.tsxsrc/components/cardInfo/expireDate/ExpireDateSection.stories.tsxsrc/components/cardInfo/expireDate/ExpireDateSection.tsxsrc/components/cardInfo/expireDate/useExpireDate.tssrc/components/cardInfo/expireDate/useExpireDateValidation.tssrc/components/cardInfo/formField/FormField.stories.tsxsrc/components/cardInfo/formField/FormField.styles.tssrc/components/cardInfo/formField/FormField.tsxsrc/components/cardInfo/isComplete.tssrc/components/cardInfo/useAutoFocus.tssrc/components/cardInfo/useCardStep.tssrc/components/cardInfo/validator.tssrc/components/cardPreview/CardPreview.stories.tsxsrc/components/cardPreview/CardPreview.styles.tssrc/components/cardPreview/CardPreview.tsxsrc/components/cardPreview/cardNumber/utils.tssrc/components/cardPreview/utils.tssrc/components/completePage/CompletePage.stories.tsxsrc/components/completePage/CompletePage.styles.tssrc/components/completePage/CompletePage.tsxsrc/components/useCardForm.tssrc/constants/constants.tssrc/utils/cardNetwork.ts
💤 Files with no reviewable changes (14)
- src/components/cardInfo/cardInfoHeader/CardInfoHeader.styles.ts
- src/components/cardInfo/cvc/CvcSection.stories.tsx
- src/components/cardInfo/cardInfoHeader/CardInfoHeader.stories.tsx
- src/components/cardInfo/formField/FormField.tsx
- src/components/cardInfo/cardNumber/useCardNumber.ts
- src/components/cardInfo/formField/FormField.styles.ts
- src/components/cardInfo/formField/FormField.stories.tsx
- src/components/cardInfo/cardInfoHeader/CardInfoHeader.tsx
- src/components/cardInfo/expireDate/ExpireDateSection.stories.tsx
- src/components/cardInfo/cardNumber/CardNumberSection.stories.tsx
- src/components/cardInfo/expireDate/ExpireDateSection.tsx
- src/components/cardInfo/cvc/CvcSection.tsx
- src/components/cardInfo/cardNumber/CardNumberSection.tsx
- src/components/cardInfo/expireDate/useExpireDate.ts
|
|
||
| 8. 카드 등록 완료 페이지 작성 | ||
|
|
||
| - ~~체크 이미지 + **\*로 시작하는 **카드가 등록되었어요 출력 |
There was a problem hiding this comment.
강조 문법(MD037) 경고를 해소해주세요.
Line 139는 강조 마커 내부 공백 때문에 markdownlint 경고가 발생합니다. 공백 없이 강조 범위를 다시 지정해 주세요.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 139-139: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 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` at line 139, Update the Markdown emphasis so there is no space
before the closing emphasis markers: change the fragment containing "**\*로 시작하는
**" in README.md to "**\*로 시작하는**" (remove the space before the closing **) to
resolve the MD037 warning about emphasis markers.
| <Label></Label> | ||
| <Trigger onClick={() => setIsOpen((prev) => !prev)}> | ||
| <span> | ||
| {cardBrand ? CARD_BRANDS[cardBrand].label : '카드사를 선택해주세요'} | ||
| </span> | ||
| <span>{isOpen ? '∧' : '∨'}</span> | ||
| </Trigger> | ||
| {isOpen && ( | ||
| <DropdownList> | ||
| {Object.entries(CARD_BRANDS).map(([brand, { label }]) => ( | ||
| <DropdownItem key={brand} onClick={() => handleSelect(brand)}> | ||
| {label} | ||
| </DropdownItem> |
There was a problem hiding this comment.
드롭다운 선택 UI가 접근성 요구를 충족하지 못합니다.
빈 Label(Line 25)은 의미가 없고, 옵션이 li + onClick만으로 동작해 키보드/스크린리더 사용자가 선택하기 어렵습니다. 트리거-목록의 ARIA 관계를 명시하고, 옵션을 포커스 가능/키보드 조작 가능 구조로 바꿔 주세요.
🤖 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 `@src/components/cardInfo/cardBrand/CardBrandField.tsx` around lines 25 - 37,
The dropdown lacks ARIA relationships and focusable options; replace the empty
Label with a meaningful label text (or give it an id and use aria-labelledby)
and make Trigger a real button or element with aria-haspopup="listbox" and
aria-expanded={isOpen}; give the DropdownList an id and role="listbox" and
aria-labelledby pointing to the label/trigger; change DropdownItem to be
focusable options (either <button> or elements with role="option" and
tabIndex=0) and wire keyboard handlers on DropdownItem to call
handleSelect(brand) on Enter/Space and support ArrowUp/ArrowDown to move focus;
ensure setIsOpen(false) runs on selection and manage focus return to Trigger;
reference Label, Trigger, DropdownList, DropdownItem, handleSelect, setIsOpen,
CARD_BRANDS, cardBrand, and isOpen when applying these changes.
| const validate = (updatedCardNumber: string[]): boolean => { | ||
| for (const segment of updatedCardNumber) { | ||
| const numberResult = validateNumber(segment); | ||
| if (!numberResult.state) { | ||
| setError(numberResult.message); | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| const lengthResult = validateCardNumberLength(updatedCardNumber); | ||
| setError(lengthResult.state ? "" : lengthResult.message); | ||
|
|
||
| return true; | ||
| }; |
There was a problem hiding this comment.
validate 함수의 반환 값 로직을 점검해 주세요
Lines 9-15에서 각 세그먼트의 숫자 검증이 실패하면 즉시 false를 반환합니다. 좋은 early return 패턴입니다!
그런데 Lines 17-20을 살펴보면:
- Line 17: 전체 카드번호 길이를 검증합니다
- Line 18: 에러 메시지를 설정합니다
- Line 20: 항상
true를 반환합니다
질문:
lengthResult.state가false(검증 실패)여도 함수가true를 반환하는게 의도된 동작인가요?- 이 반환값에 따라
CardNumberField.tsx에서 값 업데이트 여부가 결정될텐데, 길이 검증 실패 시에도 업데이트되어야 할까요? useCardPasswordValidation.ts의 동일한 패턴과 비교해보면 어떤 문제가 보이나요?
디버깅 힌트: Lines 11-12의 반환 로직과 Line 20의 반환 로직이 일관성이 있는지 확인해보세요.
🤖 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 `@src/components/cardInfo/cardNumber/useCardNumberValidation.ts` around lines 8
- 21, The validate function in useCardNumberValidation iterates segments and
early-returns false on segment validation failure (validateNumber) but always
returns true after running validateCardNumberLength and setting setError; change
this so the function returns the actual lengthResult.state (i.e., if
validateCardNumberLength fails, setError(lengthResult.message) and return
false), keeping the existing early-return behavior for validateNumber; locate
validate, validateNumber, validateCardNumberLength and setError to implement
this consistent return logic (matching the pattern used in
useCardPasswordValidation).
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const newValue = e.target.value; | ||
| if (!validate(newValue)) return; | ||
| setPassword(newValue); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check for tests covering CardPasswordField validation behavior
rg -n "CardPasswordField" --type ts --glob '*test*' --glob '*spec*' -A 10Repository: woowacourse/react-payments
Length of output: 52
🏁 Script executed:
cat -n src/components/cardInfo/cardPassword/CardPasswordField.tsxRepository: woowacourse/react-payments
Length of output: 1583
🏁 Script executed:
# Find the useCardPasswordValidation hook
fd -t f "*useCardPasswordValidation*"Repository: woowacourse/react-payments
Length of output: 377
🏁 Script executed:
# Search for the useCardPasswordValidation implementation
rg -n "useCardPasswordValidation" -A 20 --type tsRepository: woowacourse/react-payments
Length of output: 4513
🏁 Script executed:
cat -n src/components/cardInfo/cardPassword/useCardPasswordValidation.tsRepository: woowacourse/react-payments
Length of output: 796
🏁 Script executed:
# Find validateCardPasswordLength function
rg -n "validateCardPasswordLength" -A 5 --type tsRepository: woowacourse/react-payments
Length of output: 1765
🏁 Script executed:
# Find validateNumber function
rg -n "validateNumber" -A 5 --type tsRepository: woowacourse/react-payments
Length of output: 6353
validate 함수가 길이 검증 실패 시에도 항상 true를 반환합니다
Line 24의 가드 로직은 의도는 좋으나, useCardPasswordValidation 훅의 validate 함수에 버그가 있어 제대로 작동하지 않습니다.
useCardPasswordValidation.ts 라인 14-17을 보면:
validateCardPasswordLength가 실패해도 (예: "1" 입력 시)- 에러 메시지는 올바르게 설정되지만
- 라인 17에서 항상
true를 반환합니다
결과적으로 CardPasswordField의 라인 24 가드 (if (!validate(newValue)) return;)는 무시되고, 길이가 부족한 비밀번호도 상태에 저장됩니다.
수정 필요: useCardPasswordValidation.ts 라인 17을 return lengthResult.state;로 변경하세요.
🤖 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 `@src/components/cardInfo/cardPassword/CardPasswordField.tsx` around lines 22 -
26, The validate function in useCardPasswordValidation is incorrectly returning
true regardless of length check results; update the function (the validate
implementation that calls validateCardPasswordLength) to return the actual
length check state (i.e., return lengthResult.state) instead of always true so
that CardPasswordField.handleChange's guard (if (!validate(newValue)) return;)
works and setPassword is only called for valid inputs.
| const validate = (newValue: string): boolean => { | ||
| const numberResult = validateNumber(newValue); | ||
| if (!numberResult.state) { | ||
| setError(numberResult.message); | ||
| return false; | ||
| } | ||
|
|
||
| const lengthResult = validateCardPasswordLength(newValue); | ||
| setError(lengthResult.state ? "" : lengthResult.message); | ||
|
|
||
| return true; | ||
| }; |
There was a problem hiding this comment.
validate 함수의 반환 값을 다시 검토해 주세요
Lines 8-12에서 숫자 검증에 실패하면 false를 반환하고 있습니다. 좋습니다!
그런데 Lines 14-17을 보면, 길이 검증 결과를 lengthResult에 저장하고 에러 메시지는 업데이트하고 있는데요.
질문:
- Line 17에서 항상
true를 반환하는 것이 맞나요? lengthResult.state가false일 때(검증 실패)도true를 반환하면 어떤 일이 발생할까요?- 이 함수를 호출하는
CardPasswordField.tsx의 Line 24를 보면,validate가true를 반환할 때만 값을 업데이트합니다. 길이 검증에 실패했을 때도 값이 업데이트되어야 할까요?
힌트: 다른 검증 조건의 반환 로직(Lines 9-12)과 길이 검증 후의 반환 로직(Line 17)을 비교해보세요.
🤖 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 `@src/components/cardInfo/cardPassword/useCardPasswordValidation.ts` around
lines 7 - 18, The validate function incorrectly always returns true after
running validateCardPasswordLength; change its return logic to mirror the
numeric check: call validateCardPasswordLength(newValue), update setError to
lengthResult.state ? "" : lengthResult.message, and return lengthResult.state
(i.e., false when length validation fails, true when it passes) so callers like
CardPasswordField's usage (which only applies updates when validate is true)
behave correctly; keep validateNumber and its existing early-return behavior
unchanged.
| const validate = (newValue: string): boolean => { | ||
| const numberResult = validateNumber(newValue); | ||
| if (!numberResult.state) { | ||
| setError(numberResult.message); | ||
| return null; | ||
| return false; | ||
| } | ||
|
|
||
| const lengthResult = validateCvcLength(newValue); | ||
| setError(lengthResult.state ? "" : lengthResult.message); | ||
|
|
||
| return newValue; | ||
| return true; | ||
| }; |
There was a problem hiding this comment.
세 개의 검증 훅에서 동일한 패턴의 문제가 반복되고 있습니다
이 validate 함수는 useCardPasswordValidation과 useCardNumberValidation과 동일한 구조를 가지고 있는데요.
Lines 8-12에서는 숫자 검증 실패 시 false를 반환하지만, Lines 14-17에서는 길이 검증 결과와 관계없이 항상 true를 반환하고 있습니다.
질문:
- Line 17에서
return true대신return lengthResult.state를 반환해야 하는 것은 아닐까요? - 세 개의 검증 훅(
useCvcValidation,useCardPasswordValidation,useCardNumberValidation)에서 모두 같은 패턴이 보이는데, 이 부분을 함께 점검해보셨나요? - 길이 검증에 실패했을 때(예: "12"만 입력) 값이 업데이트되지 않아야 하는 것이 맞는 요구사항인가요?
힌트:
- 세 훅의
validate함수 끝부분을 비교해보세요 lengthResult.state값에 따라 반환값이 달라져야 하는지 생각해보세요- 또는 길이 검증 실패를 에러로만 표시하고 값 업데이트는 허용하는 것이 의도된 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 `@src/components/cardInfo/cvc/useCvcValidation.ts` around lines 7 - 18, The
validate function in useCvcValidation returns true regardless of the length
check result causing inconsistent behavior (it returns false on number
validation failure but always true after length validation); change the final
return from always-true to return lengthResult.state so the function reflects
the length validation outcome, and then audit the same validate implementations
in useCardPasswordValidation and useCardNumberValidation to make the same change
(or explicitly decide to allow value updates on length failures and adjust
caller UX accordingly).
| const monthResult = validateMonth(updatedExpireDate[0]); | ||
| if (!monthResult.state) { | ||
| setError(monthResult.message); | ||
| return true; | ||
| } | ||
|
|
||
| const expireDateResult = validateExpireDateNotPast(updatedExpireDate); | ||
| setError(expireDateResult.state ? "" : expireDateResult.message); | ||
|
|
||
| return true; |
There was a problem hiding this comment.
검증 실패인데 성공(true)을 반환하고 있습니다.
Line 19는 월 검증 실패 분기인데 true를 반환하고, Line 25도 최종 검증 결과와 무관하게 true를 반환합니다. 현재 호출 방식에서는 잘못된 월/만료일도 상태 업데이트가 통과됩니다. 실패 시 false, 최종 반환은 실제 검증 결과를 반영하도록 정리해 주세요.
🤖 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 `@src/components/cardInfo/expireDate/useExpireDateValidation.ts` around lines
16 - 25, The function currently returns true even when validation fails; change
the logic so validateMonth(updatedExpireDate[0]) failure sets
setError(monthResult.message) and returns false (not true), and after calling
expireDateResult = validateExpireDateNotPast(updatedExpireDate) setError to ""
on success or expireDateResult.message on failure and return
expireDateResult.state (i.e., return false on failure, true on success) instead
of always returning true; update the branches around validateMonth, setError,
and the final return to use validateMonth, validateExpireDateNotPast, setError,
and updatedExpireDate as the referenced symbols.
| if (step === 0 && isCardNumberComplete(cardForm.cardNumber.value)) setStep(1); | ||
| if (step === 1 && isCardBrandComplete(cardForm.cardBrand.value)) setStep(2); | ||
| if (step === 2 && isExpireDateComplete(cardForm.expireDate.value)) setStep(3); | ||
| if (step === 3 && isCvcComplete(cardForm.cvc.value)) setStep(4); | ||
| if (step === 4 && isAllComplete) setStep(5); | ||
| if (step === 5 && !isAllComplete) setStep(4); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: React 19에서 렌더 중 상태 업데이트 패턴 검색
# 다른 파일에서도 유사한 패턴이 있는지 확인
rg -n -C3 'useState.*setStep|setState.*if.*step' --type=ts --type=tsx -g '!node_modules'Repository: woowacourse/react-payments
Length of output: 98
🏁 Script executed:
cat -n src/components/cardInfo/useCardStep.tsRepository: woowacourse/react-payments
Length of output: 1319
렌더링 중 상태 업데이트가 발생하고 있습니다.
21~26번 줄에서 setStep()을 훅의 본문에서 직접 호출하고 있습니다. 이는 React의 렌더링 단계에서 상태를 업데이트하는 것으로, 다음과 같은 문제를 일으킵니다:
- 매번 렌더링할 때마다 상태 업데이트가 발생하여 불필요한 재렌더링 유발
- React 규칙 위반: 상태 업데이트는 이벤트 핸들러나 Effect 내에서만 수행되어야 함
- React 19에서는 더욱 엄격하게 체크됩니다
고려할 질문:
cardForm의 값이 변경될 때만step을 업데이트하려면 어떤 Hook을 사용해야 할까요?- 상태 업데이트의 의존성은 무엇이어야 할까요? (cardForm의 어떤 필드들이 변할 때 step을 다시 계산해야 할까요?)
- 이 로직을 순수하게 유지하려면 어떻게 구조화하면 좋을까요?
🤖 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 `@src/components/cardInfo/useCardStep.ts` around lines 21 - 26, The inline
calls to setStep() must be moved into a useEffect so state updates occur in an
effect instead of during render: create a useEffect that depends on the cardForm
fields used (cardForm.cardNumber.value, cardForm.cardBrand.value,
cardForm.expireDate.value, cardForm.cvc.value) and isAllComplete, compute the
correct targetStep by running the same guards (isCardNumberComplete,
isCardBrandComplete, isExpireDateComplete, isCvcComplete, isAllComplete) and
call setStep(targetStep) only if targetStep !== step to avoid redundant updates;
update references to setStep, cardForm and the predicate functions in that
effect and remove the direct setStep(...) calls from the component body.
| const state = location.state as LocationState | null; | ||
|
|
||
| if (!state) return <Navigate to="/" replace />; | ||
|
|
||
| const { cardNumberFirstSegment, cardBrand } = state; |
There was a problem hiding this comment.
라우트 state의 형태 검증이 한 단계 더 필요합니다.
Line 14~18은 null만 막고 있어서, 키 누락/타입 불일치 state가 들어오면 메시지에 undefined가 노출될 수 있습니다.
cardNumberFirstSegment와 cardBrand가 모두 문자열인지까지 확인한 뒤, 아니면 Line 16과 동일한 fallback으로 보내는 흐름을 추가해 보세요.
🤖 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 `@src/components/completePage/CompletePage.tsx` around lines 14 - 18, The route
state check only guards against null but not malformed shapes, so ensure the
state conforms to the expected LocationState shape by verifying that
state.cardNumberFirstSegment and state.cardBrand are typeof "string" before
using them; if either check fails, return the same fallback (<Navigate to="/"
replace />). Update the logic around location.state (and the LocationState type
usage) to perform these string-type checks for cardNumberFirstSegment and
cardBrand and redirect when they are missing or not strings.
| function detectCardNetworkMatch( | ||
| cardNumber: string, | ||
| ): { network: NetworkKey; matchedLength: number } | null { | ||
| for (const name of Object.keys(CARD_NETWORKS) as NetworkKey[]) { | ||
| const network = CARD_NETWORKS[name]; | ||
|
|
||
| if ('prefixes' in network) { | ||
| const matched = network.prefixes.find((prefix) => cardNumber.startsWith(prefix)); | ||
| if (matched) return { network: name, matchedLength: matched.length }; | ||
| } | ||
|
|
||
| if ('ranges' in network) { | ||
| for (const { from, to } of network.ranges) { | ||
| const digits = String(from).length; | ||
| const head = Number(cardNumber.slice(0, digits)); | ||
| if (head >= from && head <= to) return { network: name, matchedLength: digits }; | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if there are tests covering edge cases for detectCardNetworkMatch
rg -n "detectCardNetworkMatch|detectCardNetwork" --type ts --glob '*test*' --glob '*spec*' -A 5Repository: woowacourse/react-payments
Length of output: 52
🏁 Script executed:
cat -n src/utils/cardNetwork.tsRepository: woowacourse/react-payments
Length of output: 2232
🏁 Script executed:
fd -e test.ts -e spec.ts -e test.js -e spec.js | head -20Repository: woowacourse/react-payments
Length of output: 52
🏁 Script executed:
rg "detectCardNetwork" --type ts -B 2 -A 2 | head -50Repository: woowacourse/react-payments
Length of output: 2098
빈 문자열 입력 시 Range 기반 매칭의 동작을 검토해주세요.
detectCardNetworkMatch 함수가 빈 문자열을 받을 때 어떻게 동작하는지 추적해보셨나요?
Line 15의 prefixes 확인은 안전합니다 ("".startsWith(prefix)는 항상 false). 하지만 Line 22의 Range 매칭에서는 문제가 있을 수 있습니다:
Number("")는0을 반환합니다- 만약 어떤 카드 네트워크의 Range에
0이 포함되면, 빈 문자열도 매칭될 수 있습니다
현재 코드에서:
- 빈 문자열이 들어올 수 있는 상황(사용자 입력 중 등)이 있는지 확인해보세요
- 각 카드 네트워크의
from값이 어떤 범위인지 살펴보세요 (0을 포함하는지) - 필요하다면
cardNumber.length === 0일 때 early return 하는 것을 고려해보세요
또한 현재 이 함수에 대한 테스트가 없는 것으로 보이니, 엣지 케이스(빈 문자열, 1-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 `@src/utils/cardNetwork.ts` around lines 8 - 28, detectCardNetworkMatch can
falsely match empty strings via the ranges branch because Number("") === 0; add
an early guard at the start of detectCardNetworkMatch to return null for
cardNumber === "" (and consider also trimming and rejecting non-digit input) so
range checks never run on empty input, update the function that iterates
CARD_NETWORKS (detectCardNetworkMatch) to check cardNumber.length === 0 before
prefixes/ranges, and add unit tests for edge cases (empty string, 1-2 digit
numbers, non-digit characters) to prevent regressions.
eastroots92
left a comment
eastroots92
left a comment
There was a problem hiding this comment.
안녕하세요 코브! 고생 많으셨어요!
코드에 재미난 요소들이 많아 흥미롭게 코드를 보았던 것 같아요.
같이 이야기 나눠보거나 고민해보시면 좋을 부분이 있어 질문을 남겨 두었어요.
질문에 대해 답변 드려보도록 할게요.
- CARD_NETWORKS 관련
각각 장단이 있고 정답은 없는 부분이라고 생각해요.
실제로 제가 현업에서 로직을 구현할 때 matches와 같이 하는 경우, 순수 데이터만 상수로 두는 경우가 공존하는데요.
matches를 사용하는 경우는 사용하는 쪽에서 커스텀한 validate 로직이 필요한 경우, 각 속성 별로 규칙 없이 너무 다양한 패턴의 validation이 있는 경우 등등에 사용해요.
현재는 일단 코브가 잡으신 기준으로 해봐도 좋을 것 같아요.
다만 어떤 속성을 넣을 수 있는지 TS를 쓰는 만큼 타입을 잘 선언해서 관리하는 방향으로 접근해보시면 좋겠어요.
- 확인 버튼을 자식에서 부모로 올린(lift) 결정
이 부분은 정답이 없어요.
그리고 실제로 웹앱을 만들다 보면 하단에 고정된 형태로 만드는 경우가 있는데 이런 경우 코브가 하신 것 처럼 부모에 올려서 처리하기도 해요.
내부에서 처리 할 수도 있지만 매번 고정되게 하는걸 대응하는게 번거롭기도 하고, 상황별로 하단 고정된 형태에 보여져야 할 것이 달라진다면 상위에서 한번에 관리하는게 더 편한 경우도 많기 때문이에요.
이와 별개로 CardInfo 컴포넌트의 네이밍이 모호하다는 생각은 살짝 들었어요. Form이니 이 성격을 더 잘 나타내면 좋았겠다 정도?
-
Storybook 유지 보수 비용에 대한 고민
이 부분은 스토리북을 어떻게 활용할 것인가에 따라 달라요.
저의 경우 재사용 성 높은 컴포넌트(공통 컴포넌트)는 스토리북을 잘 작성하지만 단위테스트 이상의 컴포넌트에는 잘 작성하지 않아요.
만약 작성한다면 QA하는 과정에서 디자이너가 QA를 편하게 하기 위한 목적으로 간간히 적용해요.
스토리북 뿐 아니라 테스트 코드 모두 당연히 있으면 좋을 내용이지만 상황에따라선 유지보수 비용이 많이 드는 도구라고도 생각해요. 그렇기에 이걸 해서 얻을 것이 무엇인지? 명확하게 하고 사용하는 것을 추천해요.
TMI) 요즘엔 AI를 사용하다보니 테스트 코드나 스토리북은 AI에게 작성을 위임하기도 해요. -
짧은 코드도 역할에 따라 무조건 분리해야 할까
무조건 분리하지 않아도 돼요.
오히려 짧은 로직을 역할 기준으로 기계적으로 쪼개는 것은 좋은 설계가 아닐 수 있어요.
핵심은 하나의 책임을 너무 좁게 해석하지 않으셨으면 해요.
한두줄 정도의 모듈이 생긴다면 보통 투머치한 분리인 경우가 많더라구요. 더 큰 범위의 필요한 맥락의 추상화가 필요하진 않을까? 고민해보시면 좋겠어요.
단 무조건 1~2 줄 코드가 나쁘다는 건 아니에요!
정리하면, 재사용 되거나, 테스트하고 싶은 독립 로직이거나, 분리하여 네이밍을 했을 때 의도가 더 잘 드러나는 등 목적이 명확한 경우에 나누고, 만약 너무 잘게 쪼개진다면 지금 상황에 필요한 추상화 단위인지 고민해보시면 좋겠어요.
고생 많으셨습니다.
| @@ -0,0 +1,43 @@ | |||
| import { useState } from 'react'; | |||
| import { Description, Field, Label, Title } from '../CardInfo.styles'; | |||
| import { Trigger, DropdownList, DropdownItem } from './CardBrandField.styles'; | |||
There was a problem hiding this comment.
P5
현재는 여기서만 사용하니 괜찮은데 이후에 SelectBox도 추상화 해볼 여지가 있다고 생각되긴 하네요!
There was a problem hiding this comment.
시멘틱 태그로 변경하는 과정에서는 SelectBox 추상화 부분까지는 미처 고려하지 못했습니다.
현재는 시멘틱 태그 기반으로 동작하면서 자연스럽게 표준 selection 위젯을 사용하는 형태가 되었고, 그에 따라 별도의 추상화 필요성도 이전보다 줄어든 것 같습니다.
| <Field> | ||
| <Title>카드사를 선택해 주세요</Title> | ||
| <Description>현재 국내 카드사만 가능합니다</Description> | ||
| <Label></Label> |
There was a problem hiding this comment.
export const Trigger = styled.button`
cursor: pointer;
font-size: 14px;
color: rgba(172, 172, 172, 1);
- margin-top: 16px;
`;
현재는 Label에 margin-top이 들어가 있어서, UI 간격을 맞추기 위해 Label을 남겨두고 있었습니다.
다시 생각해보니, 코드를 보는 입장에서는 실제로 사용하지 않는 컴포넌트가 왜 남아있지?라는 의문이 있겠다는 생각이 들었습니다.
그래서 간격에 대한 책임은 실제로 사용하는 컴포넌트가 가지는 것이 더 자연스럽다고 판단했습니다.
Trigger는 내부에서 직접 사용되고 있는 요소이기도 해서, 해당 컴포넌트에 margin-top을 넣는 방향으로 수정했습니다.
이렇게 정리하는 방향이 맞는지 궁금합니다.
| field: ReturnType<typeof useCardForm>['cardNumber']; | ||
| } | ||
| //카드 번호를 입력할수 있는 컴포넌트 | ||
| export default function CardNumberField({ field }: Props) { |
There was a problem hiding this comment.
추가 요청
현재 Input을 입력하는 중임에도 바로 에러 안내가 뜨는게 어색하단 생각이 들었어요.
입력하다 이탈을 하면 error를 표시하는게 어색하지 않은데, 현재 입력중임에도 첫 글자를 입력하자마자 뜨는 부분에서 어색함을 느꼈던 것 같아요.
사실 큰 문제라고 생각하진 않지만 성장 목적에서 한번 고려해서 구현해 볼 수 있을까요?
There was a problem hiding this comment.
AI와 대화를 나누다 touched 패턴을 알게 되었습니다.
useAutoFocus처럼 한 가지 역할이 뚜렷한 hook으로 빼는 것이 자연스럽다고 판단해서 useBlur로 분리했습니다.
다만 역할이 뚜렷하더라도 코드가 몇 줄 되지 않을 때 굳이 hook으로 빼는 것이 맞는지
아니면 그냥 컴포넌트 안에 두는 것이나은지는 아직 판단이 서지 않습니다.
| @@ -0,0 +1,15 @@ | |||
| import { useRef } from "react"; | |||
|
|
|||
| export function useAutoFocus() { | |||
There was a problem hiding this comment.
오 재미난 접근이네요.
이 방법은 어떻게하다 떠올리시게 되었나요!?
There was a problem hiding this comment.
정말 우연히 발견하게 된 부분이었습니다.
이 기능을 가장 후순위로 미뤄두었다가 마지막에 작업하면서 보니 어? 이거 expireDate랑 cardNumber에서 동일하게 쓰이고 있네? 그냥 함수로 빼도 되겠다 라는 생각이 들었던 것 같습니다.
다만 돌이켜보면, 위 validation 코멘트에서는 2개 정도를 위해 굳이 추상화할 필요가 없다 라고 이야기했는데, 여기서는 또 분리를 진행한 점이 모순적으로 보이기도 합니다.
There was a problem hiding this comment.
다시 생각해보니 두개의 차이점이 있다면 특징이 다른 것 같습니다. useAutoFocus는 동작의 분기 있지않고
validation은 동작이 나중에는 달라지기 때문에 슈퍼함수가 될 수도 있다는 생각이 들었습니다.
현재는 validation자체가 삭제가 되서 고려해야할것이 사라졌지만 현재의 저의 생각은 이렇습니다
| import { validateCardNumberLength, validateNumber } from "../validator"; | ||
|
|
||
| //카드 번호를 정확히 입력했는지 확인하는 함수 | ||
| export function useCardNumberValidation() { |
There was a problem hiding this comment.
질문
props로 value를 넘겨받아 처리하는 방식 VS 현재처럼 validate 함수를 전달하는 방식
각각 장단이 있을 것 같은데 어떤 장단점이 있을까요?
There was a problem hiding this comment.
질문에 대해서 고민하다보니 validation 훅의 역할이 모호하다고 느껴졌어요. validate라는 이름인데 리턴값은 입력을 받아들일지 여부라서 이름과 맞지 않았어요.
validate를 걷어내고 보니 error 상태도 굳이 필요 없었어요. error는 결국 현재 값에서 파생되는 정보였고 매번 계산하면 충분했거든요.
validation 훅이 에러 메시지를 반환할 수는 있지만 메시지를 만드는 역할은 이미 validator 파일에서 하고 있어요. 그 위에 훅으로 한 번 더 감쌀 이유가 없다고 느꼈어요.
그래서 validation을 삭제 하기로 했습니다.
여기서 의문이 드는것이 컴포넌트가 너무 많은 것을 책임지고 있나? 라는 생각을 하게되었습니다.
루트는 이 컴포넌트가 너무 많은 책임을 가지고 있다고 생각하시나요?
| <InputContainer> | ||
| {segments.map((length, index) => ( | ||
| <InfoInput | ||
| key={index} |
There was a problem hiding this comment.
리액트에서 loop에 index 쓰는 것을 최대한 피하라는 이야기가 있는데요.
그 이유가 무엇일까요?
There was a problem hiding this comment.
제 방식으로 정리 해보았습니다.
리액트에서 인덱스를 key로 사용하면 안 되는 이유는 컴포넌트의 상태가 의도와 다르게 유지될 수 있기 때문입니다.
예를 들어 배열의 맨 앞에 새로운 아이템이 추가되어 기존 아이템들의 인덱스가 하나씩 밀리게 되면, 리액트는 기존의 0번 key를 가진 컴포넌트를 그대로 재사용하려고 합니다. 이 과정에서 새로 추가된 데이터에 이전 아이템의 상태(state)가 남아 있게 되고, 그 결과 개발자가 의도하지 않은 동작이 발생할 수 있습니다.
즉, key는 단순한 순서가 아니라 각 요소를 안정적으로 식별할 수 있는 값이어야 합니다.
| import { validateCvcLength, validateNumber } from "../validator"; | ||
|
|
||
| export function useCvc() { | ||
| export function useCvcValidation() { |
There was a problem hiding this comment.
P3;
useOOOValidation 에 공통된 Flow가 보이는데 useValidation 으로 추상화 하여 사용해보는 것은 어떻게 생각하시나요?
사용하는 쪽에서도 굳이 useCvcValidation 이렇게 매번 목적에 맞게 hook을 만들 필요가 있을까? 하는 생각도 있어서 여쭈어봐요!
There was a problem hiding this comment.
처음에는 저도 같은 생각으로 묶어볼까 고민했습니다. 하지만 입력 타입이 string과 string[] 두 가지로 갈리고, string[] 쪽도 카드번호/만료일이 흐름이 달라서 사실상 다 따로 묶어야 하는 상황이더라구요.
결국 2개를 위해서 추상화할 필요가 있을까? 라는 생각이 들었습니다.
| export default function CardInfo({ cardForm, step }: Props) { | ||
| return ( | ||
| <Wrapper> | ||
| <CardNumberSection field={cardForm.cardNumber} /> | ||
| <ExpireDateSection field={cardForm.expireDate} /> | ||
| <CvcSection field={cardForm.cvc} /> | ||
| {step >= 4 && <CardPasswordField field={cardForm.cardPassword} />} | ||
| {step >= 3 && <CvcField field={cardForm.cvc} />} | ||
| {step >= 2 && <ExpireDateField field={cardForm.expireDate} />} | ||
| {step >= 1 && <CardBrandField field={cardForm.cardBrand} />} | ||
| <CardNumberField field={cardForm.cardNumber} /> | ||
| </Wrapper> |
There was a problem hiding this comment.
이렇게 구현했을 때 유지보수 관점에서 우려되는 부분이 있다면 어떤 부분일까요?
There was a problem hiding this comment.
가장 먼저 떠오른 단점은 step이 늘어날수록 이 파일에 매번 한 줄씩 추가해줘야 한다는 점입니다. 단계가 늘어날 때마다 CardInfo.tsx가 계속 수정되어야 한다는 단점이 있는 것 같습니다.
추가적으로 AI에게 물어보았습니다.
- 매직 넘버: step >= 1, step >= 2처럼 숫자가 그대로 쓰여 있어서 각 단계가 어떤 의미인지 코드만 봐서는 알기 어렵습니다. 상수나 enum으로 의미를 부여하지 않으면 신규 합류자가 흐름을 파악하기 힘들 것 같아요.
- step ↔ field 매핑의 분산: 같은 매핑이 다른 곳(진행률 표시 등)에서 필요해지면 동기화 이슈가 생길 수 있습니다.
- step >= N 누적 조건의 경직성: "특정 단계에서는 일부 필드만 보인다" 같은 비누적 요구사항이 들어오면 패턴 자체를 갈아야 합니다.
이렇게 3개가 더 있다고 했습니다.
현재 상태에서는 3번이 가장 눈에 들어왔습니다.
CardInfo는 카드 입력 필드들의 묶음을 렌더링하는 역할이라 '정보'보다 '폼 필드'가 의미에 부합. 폴더·파일·컴포넌트명을 일관되게 변경.
validate가 입력 차단(boolean 반환)과 에러 갱신(setError) 두 책임을 동시에 가지고 있었고, validate를 걷어내고 보면 error는 현재 값에서 파생되는 정보라 useState로 들고 있을 이유가 없었음. validator.ts에 isNumericInput과 getXxxError를 추가하고 컴포넌트에서 직접 호출하는 방식으로 변경. useXxxValidation 훅 4개 삭제.
입력 첫 글자부터 에러가 깜빡이는 UX 문제를 해결. 사용자가 필드를 한 번 떠났을 때(blur)부터 에러 표시를 시작하고, 그 후엔 입력 변경에 따라 실시간 갱신. touched 상태와 handleBlur는 useBlur 훅으로 분리해 4개 필드(CardNumber, ExpireDate, Cvc, CardPassword)가 공유.
2 participants
🎯 페이먼츠
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
2단계
3단계
idle | loading | success | error네 가지로 명시적으로 관리한다.🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
이번에 프로젝트를 진행하면서 가장 많이 고민했던 키워드는 단순화였어요. step2를 시작할 때 컴포넌트 하나당 역할 하나만 맡기자는 다짐을 주석으로 적어놓고 시작했거든요. 처음부터 너무 쪼개기보다는, 일단 추상화된 것들을 다 풀어서 구현해 놓고 나중에 나누자는 전략이었어요.
그런데 코드를 짜다 보니 제가 주석에 적어둔 역할이랑 실제 코드의 모습이 따로 노는 순간들이 꽤 있더라고요.
가장 먼저 눈에 띈 건 validator.ts 파일이었어요. 처음엔 검증 함수들을 다 한곳에 모아뒀는데, 가만히 보니 얘네가 성격이 다른 두 그룹으로 나뉘더라고요. 하나는 { state, message }를 넘겨주는 validate 계열이고, 다른 하나는 boolean만 뱉는 isComplete 계열이었죠. 둘 다 검증 도메인이긴 해도, 하나는 UI에 에러를 보여주려고 쓰는 거고 다른 하나는 다음 단계로 넘어갈 수 있는지 판단하려고 쓰는 거라 목적이랑 호출자 자체가 완전히 달랐어요. 결국 validator.ts와 isComplete.ts로 파일을 분리했는데, 이 과정을 통해서 반환 타입이 다르다는 게 곧 책임이 다르다는 힌트가 된다는 걸 배운 것 같아요.
2) 이번 리뷰를 통해 논의하고 싶은 부분
카드 네트워크 판별 규칙을 정리하면서 두 가지 방법 사이에서 고민이 좀 있었습니다.
하나는 항목 안에 matches 같은 함수를 넣어서 룰을 데이터와 함께 캡슐화하는 방식이고, 다른 하나는 prefixes나 ranges 같은 순수 데이터만 상수 파일에 두고 비교 로직은 별도 함수에서 처리하는 방식이었어요.
저는 고민 끝에 후자를 택했습니다. JS 문법상 const로 묶을 수는 있지만, 제가 생각하는 상수는 변하지 않는 데이터여야 한다고 판단했거든요. 함수가 들어가는 순간 이 정신에서 조금 멀어진다는 기분이 들었습니다.
다만, 나중에 카드사 판별 규칙이 정규식처럼 새로운 형태로 추가된다면 데이터 구조 자체를 뜯어고쳐야 해서 함수를 두는 쪽이 더 유연할 것 같긴 합니다. 이런 트레이드오프 상황에서 함수를 상수에 포함하는 것을 어느 정도까지 허용해도 괜찮다고 보시는지 리뷰어님의 의견이 궁금합니다.
확인 버튼을 카드 프레임 전체 폭에 맞춰 깔리게 디자인하려다 보니, 원래 CardInfo 안에 있던 버튼을 PaymentWidget으로 끌어올리게 되었습니다. 사실 CSS 트릭(음수 margin이나 absolute)을 쓰면 자식 컴포넌트에서도 부모 영역을 침범할 수는 있었지만, 자식이 부모의 레이아웃 상세 정보(margin 등)를 알아야 한다는 결합도가 마음에 걸려 리프팅을 선택했습니다.
대신 그 비용으로 useCardStep, useNavigate, handleConfirm 같은 로직들이 전부 PaymentWidget으로 따라 올라가면서 부모 컴포넌트가 좀 무거워졌어요. CardInfo가 단순해진 대신 PaymentWidget의 책임이 커진 셈인데, 이런 형태의 구조가 적절한 균형이었는지, 아니면 별도의 컴포넌트를 두는 게 더 나은 선택이었는지 궁금합니다.
리팩토링 과정에서 코드가 계속 변하다 보니 스토리북을 매번 수정하는 게 부담으로 다가왔습니다. Props나 Hook 의존성이 바뀔 때마다 args랑 decorator를 일일이 손봐야 하니까요.
처음 컴포넌트를 만들 땐 큰 도움이 됐지만, 이후 단계에서는 유지 비용이 너무 커져서 결국 일부 스토리가 깨진 채 방치되기도 했습니다. 루트 님은 어떤 기준으로 스토리북을 유지하시나요? 모든 컴포넌트에 스토리를 두는 게 맞을지, 아니면 특정 수준의 컴포넌트에만 작성하는 게 합리적일지 궁금합니다.
이번 step2를 시작할 때 한 컴포넌트나 훅에는 하나의 역할만 담고, 그걸 주석으로 나타내자라는 전략을 세웠습니다. 그런데 막상 코드를 짜다 보니 4~5줄 정도 되는 짧은 로직이 주석에 적어둔 역할에서 아주 살짝 벗어나는 경우들이 생기더라고요.
예를 들어 useCardNumberValidation은 검증 확인 훅이라고 정의했지만, 내부에는 검증 호출뿐만 아니라 에러 상태 관리도 같이 들어있었습니다. 엄격하게 따지면 두 가지 책임이지만, 이걸 또 쪼개자니 파일만 늘어나고 한두 줄짜리 모듈이 만들어 지더라고요.
원칙을 지키자고 코드를 잘게 쪼개면 오히려 전체 흐름을 추적하기 어려워지는 것 같고, 그대로 두자니 주석이 거짓말을 하는 것 같아 찝찝합니다. 보통 어느 정도 수준까지를 하나의 책임으로 보고 묶어두시는지, 분리 기준의 최소 단위에 대한 루트 님의 생각이 궁금합니다.
✅ 리뷰어 체크 포인트
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*를 사용했는가?