[페이먼츠 3단계 - MSW & Async & Testing] 유월(김예안) 미션 제출합니다. #570
Conversation
기존 ServiceLayout -> FormLayout 네이밍 변경
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Walkthrough이 PR은 React 결제 시스템에 카드 관리 기능을 추가하고 테스트 인프라를 구축합니다. MSW(Mock Service Worker)로 브라우저와 Node 양쪽에서 API를 모킹하고, Vitest와 Testing Library로 통합 테스트를 작성합니다. 카드 등록, 목록 조회, 삭제 API를 구현하며, useQuery 훅으로 비동기 데이터 조회를 관리합니다. 라우팅을 중첩 레이아웃 기반으로 재구성하여 FormLayout과 CardListLayout으로 페이지를 감싸고, 타입 import 경로를 통합합니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~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)
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 |
degurii
left a comment
degurii
left a comment
There was a problem hiding this comment.
안녕하세요 유월! 3단계 과제 진행하시느라 고생 많으셨습니다.
테스트는 로컬에서 확인했고 잘 통과된 걸로 확인했어요. 코드에서 바로 짚을 부분은 인라인 코멘트로 남기고, PR description에 적어주신 질문 중심으로 답변드리겠습니다.
- 기능 명세
이번 미션처럼 서비스 규모가 작은 경우, 대부분의 기능이 한두개 도메인이랑만 연결돼있어요. 그래서 오히려 도메인이란 걸 정의하기가 더 어려울 수 있습니다.
도메인을 어떻게 정의하느냐는 꽤 어려운 얘기에요. 사람마다 생각도 달라서 다르게 받아들일 수 있을 것 같구요. 우선 저도 유월이 적어주신 것처럼 "관련된 것들을 같은 관심사로 묶어주는 상위 개념"에 가깝게 생각하는 편입니다. 다만 기능 명세를 작성할 때 처음부터 도메인별로 나누려고 하지는 않는 것 같아요. 현업에서는 기획서가 화면이나 기능 단위로 탄탄하게 잘 나뉘어 있는 경우가 많아서, 저같은 경우는 그 단위대로 정리하는 일이 더 많았던 것 같아요.
기능 명세 폼 자체는 크게 미비한 부분은 없어보여요. 다만 별개의 이야기인데, 태스크 크기는 애매하면 조금 더 잘게 쪼개는 편을 추천드려요. 잘게 쪼개진 태스크를 나중에 하나로 묶는 건 쉬운데, 처음부터 크게 잡은 태스크를 작업 중간에 다시 쪼개는 건 생각보다 비용이 큰 작업이거든요.
추가로 MD나 story point처럼 작업 일정을 추정하는 칸을 하나 넣어보면 좋을 것 같아요. 처음에는 예측이 거의 안 맞을 가능성이 높은데, 그래도 피처별 일정을 예측하고 개발이 끝난 뒤에 실제 걸린 시간이랑 비교해보는 연습이 중요하다고 생각해요. 사람은 생각보다 본인 능력을 과대평가하기 때문에, 이런 훈련을 거치지 않으면 자기도 모르게 일정이 점점 지연되는 일이 발생할 수 있어요. 이런 경험이 쌓이면 내가 어느 정도 크기의 일을 얼마 만에 끝낼 수 있는지 점점 정교한 감이 쌓이게 됩니다.
- api 인스턴스
- 객체 리터럴 vs 팩토리 함수
객체 리터럴이냐 팩토리 함수냐같이 구현방식에 대해서는 그때그때 상황에 맞게 고르면 된다고 생각해요. 그냥 HTTP 메서드별로 함수를 만들어서 간단하게 export할 수도 있고, class로 만들 수도 있고, 공통 처리를 담당하는 함수 하나만 따로 만들 수도 있어요. 중요한 건 구현 방식 자체가 아니라 지금 코드에서 필요한 처리가 무엇인지, 그리고 그 구조가 유지보수에 실제로 도움이 되는지라고 생각합니다.
좀 눈에 밟혔던게, "추후 baseURL이나 헤더가 다른 인스턴스를 여러 개 다뤄야 할 경우"를 걱정하신 부분인데요. 요부분은 조금 조심하면 좋겠습니다. 아직 오지 않은 복잡한 상황을 먼저 가정해서 구조를 키우면 실제 문제보다 코드 복잡도만 먼저 커질 수 있어요. 그 상황이 언제올지는 아무도 모르고, 그냥 쓸데없는 복잡도만 높아진 코드가 될 수 있어요. YAGNI 관점에서 보면 지금은 base URL 하나, 클라이언트 하나만 있고 내부 상태도 없는 상황이라 객체 리터럴 방식으로도 충분해 보입니다. 나중에 정말 여러 base URL이나 인증 정책이 생겼을 때 팩토리 함수로 옮겨도 늦지 않다고 생각해요.
- 에러 처리 구조
에러 처리 쪽은 지금 고민하신 방향이 맞아요. throw new Error()처럼 처리하면 요청이 실패했다는 사실만 남고, 호출부에서 어떤 실패인지 판단할 재료가 사라져요.
저라면 공통 요청 함수에서는 실패 응답을 파싱해서 status와 서버가 내려준 code, message를 보존한 에러로 던지는 정도까지 맡길 것 같아요. 이 방식은 일반적 관행에 가까운 선택입니다. 예를 들면 ApiError extends Error를 두고 status, code, payload 같은 값을 함께 들고 있게 하면, 호출부에서는 error instanceof ApiError로 확인한 뒤 401은 인증 처리, 404는 빈 상태, 400은 입력 에러 표시처럼 화면별 정책을 나눌 수 있어요.
다만 공통 요청 함수가 어떤 화면으로 이동할지, 어떤 문구를 어디에 보여줄지까지 결정할 필요는 없다고 생각해요. 그건 요청 계층보다 화면 쪽 정책에 가깝습니다. 그래서 지금 단계에서는 공통부가 실패 응답의 정보를 잃지 않고 구조화해서 넘기는 역할까지만 맡고, 실제 사용자 안내나 상태 변경은 호출부에 남기는 쪽이 유지보수하기 좋아 보여요.
- 응답 파싱 로직
응답 파싱은 지금처럼 204 응답처럼 body가 비어 있을 수 있는 환경이라면 res.text()로 먼저 읽고 빈 문자열을 처리하는 방식도 충분히 합리적이라 생각합니다. 근데 사실 axios는 기본 설정에서 응답을 알아서 잘 파싱해주는 편이라 이런 고민을 덜 하긴 하지만 fetch는 res.json()을 호출할지, res.text()로 먼저 읽을지 직접 정해야하니 고민될 수 있는 포인트라 생각해요. 그리고 저는 이런 류의 질문에 항상 같은 답을 하는데, 모든 상황을 관통하는 일반적인 best practice는 없다고 생각해요. 그저 상황에 맞춰서 복잡도와 유지보수성의 트레이드 오프 사이에서 잘 줄타기하는 게 능력이라고 생각합니다.
- useQuery
- 전반적인 설계 방향
전반적인 설계 방향은 적절해 보여요. 지금 스펙이 아주 복잡한 서버 상태 관리까지 요구하는 것은 아니라서, status 하나에서 isLoading, isSuccess, isError를 파생시키는 정도면 충분한 것 같습니다. 그리고 널리 쓰이는 라이브러리의 인터페이스를 차용한 건 좋은 선택이었다 생각해요. 생태계가 성숙해지면서 점점 사용하기 좋은 방향으로 다듬어졌을 확률이 높거든요. 추가로 직접 만든 훅이지만, 익숙한 인터페이스이기 때문에 사용하는 쪽에서 더 예측하기 쉬워진다는 장점도 있고요.
- 리패칭 방식
refetchKey를 증가시켜 effect를 다시 실행하는 방식도 지금 규모에서는 충분히 이해 가능한 방식이에요. 꼭 더 일반화해야 하는 상황은 아니라고 봐요. 다만 유월이 말한 것처럼 우회적으로 느껴진다면, 요청 실행 로직을 함수로 분리하고, 최초 mount 때, refetch때 같은 함수를 직접 호출하게 만드는 방향을 생각해볼 수 있을 것 같아요. 또는 query key랑 query function을 분리해서 어떤 요청인지와 어떻게 요청할지를 명시하는 방식도 고려해볼 수 있을 것 같습니다.
- race condition 방어
cancelled 플래그는 이전 요청 결과가 뒤늦게 도착했을 때 화면 상태를 덮어쓰지 않도록 막는 기본 방어로는 괜찮아요. 다만 실제 네트워크 요청 자체를 중단하는 것은 아니라서, 요청 취소까지 필요하다면 AbortController를 같이 고려할 수 있어요. 이번 코드에서는 느린 응답이나 unmount 이후 상태 업데이트를 막는 정도가 목적이라면 현재 방식도 충분히 납득됩니다.
별도로, 인라인 코멘트에 남겻듯 실패한 뒤 다시 성공했을 때 이전 error를 어떻게 정리할지와 queryFn이 매 렌더마다 새로 만들어지는 경우를 어떻게 볼지는 같이 점검하면 좋겠어요. 지금 사용처에서는 모듈에 정의된 함수를 넘기고 있어서 바로 문제 되지는 않지만, 훅을 공용으로 키울수록 사용 정책이나 인터페이스를 조금 더 명확히 해두는 편이 좋다고 생각해요.
- 통합 테스트
테스트 코드 작성 시점은 팀에서 테스트를 어느 정도 중요하게 보는지에 따라 달라질 수 있다고 생각해요. 제 기준으로는 기능을 다 만든 뒤 한참 나중에 몰아서 붙이기보다는, 하나의 피처 리스트를 끝낼 때 성공 케이스와 중요한 실패 케이스를 함께 포함하는 편이 좋다고 생각해요.
우테코 과제는 여러 기능이 하나의 PR에 묶일 수 있어서 PR 단위와 개발 단위가 항상 같지는 않을 수 있어요. 그래도 구현할 때는 작은 기능 단위로 나눠서, 이 동작은 어떤 테스트까지 있으면 완료로 볼 수 있을까?를 같이 생각해봐도 좋을 것 같아요.
통합 테스트는 여러 단위를 실제 사용 방식에 가깝게 조합해서 검증하는 테스트라고 보면 될 것 같아요. 프론트엔드에서는 보통 네트워크 경계만 모킹하고, 컴포넌트, 훅, 라우터는 실제로 실행시켜서 사용자가 보는 결과를 확인하는 방식에 가깝습니다.
그 기준에서 보면 유월이 작성한 테스트는 통합 테스트 범위에 잘 맞는다고 생각해요. 네트워크 경계는 MSW로 대체하고, 페이지를 테스트용 라우터 안에서 렌더링한 뒤 사용자 입력, 클릭, 화면 전환 결과를 확인하고 있으니까요.
여기서 더 보완한다면 이미 화면 동작이 정해진 케이스부터 테스트로 닫아보면 좋을 것 같아요. 지금은 다시 시도 버튼 존재는 확인하고 있는데, 클릭했을 때 목록 요청이 다시 실행되는지는 아직 확인하지 않고 있거든요. 요런 부분에 대한 테스트를 추가해봐도 좋을 것 같아요.
| await registerCard({ | ||
| number: cardNumbers.join(''), | ||
| expirationDate: `${expirationDate['month']}/${expirationDate['year']}`, | ||
| cvc, | ||
| issuerCode: issuer.issuerCode, | ||
| }); |
There was a problem hiding this comment.
[권장]
공통 요청 함수는 실패 응답에서 예외를 던지는데, 등록/삭제 호출부에서는 이 예외를 따로 처리하지 않고 있어요. 그래서 등록이나 삭제 요청이 실패하면 사용자는 실패 여부를 알기 어렵고, 화면도 어느 상태인지 정리되지 않은 채로 남을 수 있습니다.
목록 조회에서는 실패 상태를 별도로 다루고 있으니 등록/삭제 요청에서도 실패했을 때 사용자에게 어떻게 안내할지 같이 정리해보면 좋을 것 같아요.
There was a problem hiding this comment.
말씀하신 내용에 동의합니다!
다만 해당 에러를 필드 간 에러 메시지 영역에 노출시키려면 현재 InputFieldForm에서 관리 중인 에러 메시지 상태 구조를 변경해야 합니다. 이는 영향 범위가 큰 리팩토링이라 판단되어 추후 구조적인 개선과 함께 리팩토링 할 예정입니다.
| http.post('*/cards', async ({ request }) => { | ||
| const body = (await request.json()) as RegisterCardRequest; | ||
| const created = cardDB.add(body); | ||
| const response: RegisterCardResponse = { id: created.id }; | ||
| return HttpResponse.json(response, { status: 201 }); | ||
| }), |
There was a problem hiding this comment.
[필수]
PR description에서 API 에러 처리 구조를 고민해주셨는데, 지금 등록 요청은 들어온 값을 그대로 저장하고 항상 성공 응답을 반환하고 있어요. 그래서 카드 등록에서 클라이언트가 받을 수 있는 실패 응답 형태가 코드에 드러나지 않은 상태에요.
이번 단계에서는 서버에 보낼 요청과 받을 응답의 형태를 설계해보는 것이 목표여서, 대표적인 400 응답 하나는 handler나 테스트 override에 드러내주면 좋겠습니다. 그래야 공통 요청 함수에서 status, code, message를 어떤 형태로 넘길지, 호출부가 사용자에게 어떻게 보여줄지를 실제 코드 기준으로 판단이 가능해질 것 같아요.
There was a problem hiding this comment.
요구사항에 맞도록 에러 검증 로직 및 반환 형태 정의했습니다!
| setData(res); | ||
| setStatus('success'); |
There was a problem hiding this comment.
[권장]
실패한 뒤 다시 조회에 성공해도 이전 error 값을 초기화하지 않고 있어요. 지금 화면에서는 isError일 때만 에러 화면을 보여주기 때문에 크게 드러나진 않지만, 훅 반환값만 보면 success 상태에서도 이전 error객체가 남아있을 것 같아요. 요부분 정리해주는 코드가 필요해보입니다.
추가로, 이 훅은 쿼리 함수가 렌더링마다 새로 만들어지지 않는다는 전제로 구현됐어요. 지금 사용처처럼 정의된 함수를 넘기면 이슈가 없지만, 추후 인라인 함수를 생성해서 넘기는 식으로 사용하게 되면 반복 요청이 발생할 수 있습니다. 관련해서 어떤 방식으로 사용 규칙을 잡을지, 혹은 훅 인터페이스를 어떻게 보완할지 같이 생각해보시면 좋을 것 같아요.
There was a problem hiding this comment.
실패한 뒤 다시 조회에 성공해도 이전 error 값을 초기화하지 않고 있어요.
해당 케이스는 고려를 못했네요! 성공 시 에러 상태 초기화해주는 코드 추가했습니다
try {
..
setData(res);
setStatus('success');
+ setError(null)
} catch (err) {
..
}
추가로, 이 훅은 쿼리 함수가 렌더링마다 새로 만들어지지 않는다는 전제로 구현됐어요. 지금 사용처처럼 정의된 함수를 넘기면 이슈가 없지만, 추후 인라인 함수를 생성해서 넘기는 식으로 사용하게 되면 반복 요청이 발생할 수 있습니다. 관련해서 어떤 방식으로 사용 규칙을 잡을지, 혹은 훅 인터페이스를 어떻게 보완할지 같이 생각해보시면 좋을 것 같아요.
const {..} = = useQuery({ queryFn: () => getCardList(userId) });말씀하신대로 api 함수를 인라인으로 넘겨줘야 하는 케이스가 생기면, 리렌더링마다 queryFn이 새 참조로 생성되어 effect가 재실행되고 무한 요청이 발생합니다.
queryFn이 deps에 포함되는 구조가 주 원인이라고 파악했습니다.
해결 방향으로 세 가지를 고민했습니다.
1. deps에 queryFn 유지 + 호출부에서 queryFn을 useCallback으로 래핑하여 주입
무한 렌더링은 막을 수 있지만, 사용 책임이 호출부로 넘어가 매번 useCallback을 강제해야 한다는 점이 부담입니다.
2. queryKey 패턴 도입
명시적인 인터페이스로 해결할 수 있지만, 현재 규모에서 도입하기에는 다소 오버 엔지니어링이라고 판단했습니다. (YAGNI)
3. deps에서 queryFn 제외 +패칭 로직 useEffectEvent로 분리 <- 채택
queryFn은 "호출 시점에는 항상 최신값을 써야 하지만, 그 변경 자체가 refetch를 트리거해서는 안 되는" 값입니다. 이는 React 공식 문서가 useEffectEvent를 설명하는 정의와 일치한다고 봤습니다.
별도의 함수로 분리하는 과정에서 발생하는 lint 경고(set-state-in-effect)는 모두 false positive로 판단해 주석과 함께 disable 처리했습니다.
+ // refetchKey만 deps에 포함되기 때문에 cascade render가 구조적으로 발생하지 않음.
+ // eslint-disable-next-line react-hooks/set-state-in-effect
+ runQuery(signal); lint 해결 과정에서 같은 이슈를 고민한 프론트엔드 크루 레스의 글을 참고했습니다.
다만 한 가지 의문이 남는 부분이 있습니다. queryFn은 "최신값을 읽되 변경이 effect 재실행을 트리거해서는 안 되는" 값이라 useEffectEvent의 사용 기준에 부합하지만, 나머지 상태(status, data, error)들은 훅 내부에서 이미 실시간으로 반영되고 있는 값들입니다.
성격이 다른 두 종류의 로직을 하나의 useEffectEvent로 묶는 것이 적절한지에 대한 확신이 서지 않습니다.
문제 해결의 흐름의 논리와 마지막 의문점에 대한 데구리의 의견이 궁금합니다!
There was a problem hiding this comment.
문제 원인을 queryFn 참조가 deps에 들어가면서 effect가 반복 실행될 수 있는 구조로 잘 파악해주신 것 같아요. 호출부에서 useCallback으로 감싸는 방법도 가능하지만, 그렇게 하면 이 훅을 쓰는 쪽이 매번 참조 안정성을 신경 써야 하니 말씀하신 부담도 이해됩니다. 지금 단계에서 queryKey까지 도입하지 않고 useEffectEvent로 훅 내부에서 다루는 선택도 현재 요구사항에서는 충분히 납득되는 선택이에요.
질문 주신 "상태 갱신까지 같은 useEffectEvent 안에 있어도 되는가"에 답하면, 지금 코드에서는 크게 어색하지 않다고 생각해요. runQuery가 단순히 최신 queryFn만 읽는 함수라기보다, 요청을 시작하고 응답 결과를 상태에 반영하는 하나의 작업 단위에 가깝다고 생각하거든요.
현재 사용처는 getCardList처럼 요청 대상이 고정된 조회이고, 다시 불러와야 하는 시점도 삭제 후나 에러 화면에서 refetch로 명시하고 있어요. 이 정도 범위라면 queryFn 변경을 자동 재요청 기준으로 삼지 않아도 충분해보입니다. 대신 나중에 이 훅을 공용 훅으로 고도화한다고 했을 때, 특정 값이 바뀌었을 때 자동으로 다시 요청해야 하는 상황이 생긴다면 그때는 queryKey나 명시적인 deps처럼 재요청 기준을 훅 인터페이스에 드러내는 방향도 고려해보면 좋을 것 같아요.
| icon={<img src={errorIcon} alt="error-icon" />} | ||
| title="카드 목록을 불러올 수 없어요" | ||
| description="잠시 후 다시 시도해 주세요." | ||
| action={{ label: '다시 시도', onClick: () => navigate(0) }} |
There was a problem hiding this comment.
[권장]
목록조회 실패 화면의 다시 시도가 페이지 전체 리로드로 처리돼있어요. 페이지 리로드는 앱 전체 상태를 초기화하는 강한 액션이고, 목록 요청만 다시 보내는 부분 재시도와는 성격이 다르다고 생각합니다.
useQuery에서는 요청만 다시 실행할 수 있는 refetch를 반환하고 있으니, 이번 단계에서 직접 만든 훅 기능을 살리려면 에러 화면의 재시도 로직도 요걸 이용하는 편이 좋아 보여요.
혹시 의도적으로 부분 페칭 대신 페이지 리로드를 택한 이유가 있다면 그 기준도 궁금합니다.
There was a problem hiding this comment.
처음에는 에러 상태에서는 어디서 문제가 발생했는지 특정할 수 없으므로, 부분 패칭보다 페이지 전체를 새로고침해 처음부터 다시 받아오는 편이 더 안전하다고 판단했습니다.
하지만 부분 패칭과 비용을 비교해보니 페이지 리로드는 다음과 같은 점에서 비효율적이었습니다.
- JS 번들 재실행, 모든 컴포넌트 재마운트, 전역 상태·라우터 상태 초기화 등 실패한
쿼리 하나를 재시도하기 위해 치르는 비용이 지나치게 큼 - 화면 깜빡임이 발생 -> UX 측면에서도 손해
또한 에러 복구를 페이지 리로드로 처리하는 방식은 SPA의 장점(부분 갱신, 상태 유지)을 포기하는 선택이라는 생각이 들었습니다.
따라서 실패 지점이 명확한 쿼리에 한해서는 refetch로 해당 요청만 재시도하는 것이 더 적절하다고 판단했습니다.
| icon={<EmptyCard />} | ||
| title="등록된 카드가 없습니다" | ||
| description="아래 버튼을 눌러 첫 카드를 등록해보세요" | ||
| action={{ label: '카드 추가하기', onClick: () => navigate('/registration/completion') }} |
There was a problem hiding this comment.
[제안]
카드 추가하기 버튼은 카드 등록을 시작하는 버튼으로 보이는데, 현재는 /registration/completion으로 이동한 뒤에 완료 페이지의 guard를 거쳐서 /registration으로 다시 이동하고 있어요.
결과적으로 카드 등록 페이지에 도착하긴 하지만 버튼이 바로 등록 페이지로 이동시키지 않고 완료 페이지의 redirect에 기대는 구조라 의도가 조금 덜 드러나는 것 같아요. 버튼의 목적이 카드 등록 시작이라면 /registration으로 바로 이동시키는 편이 더 자연스러워 보입니다.
같은 이동 경로가 목록 하단의 + 카드 추가 버튼에도 사용되고 있어서 같이 생각해보면 좋을 것 같아요.
There was a problem hiding this comment.
말씀해주신대로 결과적으로는 동작하지만 혼동을 유발할 수도 있는 명확하지 않은 코드인 것 같네요! (코드를 짤 당시에 어떤 의도로 짠 것인지는 기억이 나지는 않습니다..)
말씀해주신 내용 반영했습니다
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
src/components/Card/CardList/FabllbackView.tsx (1)
47-51: ⚡ Quick win설명 텍스트는
strong대신 일반 텍스트 요소를 쓰는 게 맞습니다.Line 47의 힌트 문구는 강조 의미가 아니라 안내 문구라서, 시맨틱상
p/span이 더 적절합니다.🤖 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/Card/CardList/FabllbackView.tsx` around lines 47 - 51, HintText currently uses a semantic <strong> element but should be a neutral text element; change the styled component HintText from styled.strong to a non-semantic inline/paragraph element (e.g., styled.span or styled.p) and remove any implicit bold semantics (ensure font-weight remains 400 as in the CSS). Update the component definition named HintText so it renders as a neutral text node for accessibility and correct semantics.src/mocks/handlers.ts (1)
28-31: ⚡ Quick winCVC 검증이 특정 값(000) 차단에만 묶여 있어 규칙 누락 가능성이 큽니다.
현재 조건이면
'000'외의 비정상 CVC 형식이 통과할 수 있습니다. 폼에서 쓰는 CVC 규칙(자릿수/숫자 여부)과 동일한 기준을 핸들러에서도 재사용할지 점검해보는 게 좋겠습니다. 어떤 입력들을 400으로 고정할지 먼저 케이스를 정리해보실래요?🤖 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/mocks/handlers.ts` around lines 28 - 31, validateRegisterCard currently only rejects CVC '000', allowing other invalid CVCs through; update validation to enforce the same CVC rules used by the form (numeric-only and correct length) instead of the hardcoded '000' check. Modify validateRegisterCard to call or reuse a shared validator (or implement a helper) that verifies body.cvc matches /^[0-9]{3,4}$/ (or your project's CVC rule) and return errorResponse(400, 'INVALID_CVC') on failure; keep existing checks for isInvalidCardNumber and isInvalidExpirationDate and ensure the function accepts RegisterCardRequest inputs and uses the same errorResponse semantics.
🤖 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 `@src/__tests__/CardListPage.test.tsx`:
- Line 29: Replace the hardcoded PAN strings used in the test fixtures with
non-sensitive tokenized or masked values: find the test card objects in
CardListPage.test.tsx that set the number property (the literal 'number' fields
at the two occurrences flagged) and substitute a tokenized dummy (e.g. a "tok_"
token) or a masked PAN (e.g. 4111********1111) or generate them via a fixture
helper (e.g. tokenizePan/maskPan) so tests no longer contain real PAN-like
strings; update both occurrences consistently and ensure any assertions that
depend on the masked/tokenized form are adjusted accordingly.
In `@src/apis/api.ts`:
- Around line 42-47: The post helper in api.post currently JSON.stringify(body)
but doesn't ensure Content-Type: application/json and Accept: application/json;
update post to merge default headers into options.headers when calling request
(preserve any user-supplied headers and do not overwrite them), e.g., set
defaults { 'Content-Type': 'application/json', Accept: 'application/json' } and
merge with options.headers before passing the options object to
request<TResponse>, keeping the method: 'POST' and body: JSON.stringify(body)
as-is.
- Line 1: The BASE_URL constant in src/apis/api.ts uses
import.meta.env.VITE_BASE_URL while other places read import.meta.env.BASE_URL,
which can cause undefined paths; update the module to consistently read the
Vite-exposed variable and provide a safe fallback/validation: resolve the base
value from import.meta.env.VITE_BASE_URL || import.meta.env.BASE_URL || "/" (or
a configured default), validate it is not empty, and ensure any URL-building
logic (e.g., the BASE_URL constant and functions that construct
`${BASE_URL}${path}`) uses this validated value or throws/logs a clear error so
mockServiceWorker and card icon paths don’t become "undefined/…". Ensure other
modules (src/main.tsx and src/constants/cards.ts) reference the same validated
source or an exported getter to keep behavior consistent.
In `@src/components/Card/CardList/FabllbackView.tsx`:
- Line 20: The RegisterButton in FallbackView is missing an explicit type
attribute which can cause unintended form submissions; update the JSX where
RegisterButton is rendered (the instance using action.onClick and action.label
inside FallbackView) to include type="button" and do the same for the other
RegisterButton occurrences referenced (the block around lines 53-61) so these
action-trigger buttons cannot accidentally submit surrounding forms.
In `@src/components/Card/CardList/RegisteredCardItem.tsx`:
- Around line 13-23: RegisteredCardItem currently assumes issuer exists which
can be undefined; replace the filter with .find on CARD_ISSUER_CONFIG and add a
null-guard or default issuer object to prevent render crashes. Specifically,
update the lookup (use CARD_ISSUER_CONFIG.find(...) to get issuer), then if
issuer is falsy set a sensible fallback (e.g., { name: 'Unknown', color: '`#ccc`',
issuerCode: data.issuerCode }) or short-circuit render a placeholder; ensure the
JSX uses the safe issuer (Issuer $color, Name, etc.) so accessing
issuer.color/name never throws. Also keep formatCardNumber(data.number) usage
unchanged but ensure the component still renders safely when issuer is missing.
In `@src/components/Card/CardList/SkeletonCardList.tsx`:
- Around line 24-33: The Container is a div while Wrapper is a list item
(styled.li), breaking list semantics; update the markup in SkeletonCardList.tsx
so the parent matches the li children — either change Container (styled.div) to
styled.ul (keep Wrapper as styled.li) or change Wrapper (styled.li) to
styled.div (keep Container as styled.div); pick the option that preserves
intended semantics and styles, and adjust any CSS selectors or accessibility
attributes accordingly.
In `@src/components/Form/PaymentForm.tsx`:
- Around line 110-119: The code reads issuer from CARD_ISSUER_CONFIG using
cardIssuer and then unconditionally uses issuer.issuerCode in registerCard; if
cardIssuer is null the filter returns undefined and causes a runtime error. In
handleSubmit (or before calling registerCard) validate that issuer (result of
Object.values(CARD_ISSUER_CONFIG).filter(...)[0]) exists (or that cardIssuer is
non-null), and if not, abort the submit flow and surface an appropriate user
error/notification; only pass issuer.issuerCode into registerCard when issuer is
defined. Ensure you reference the existing symbols: CARD_ISSUER_CONFIG,
cardIssuer, issuer, handleSubmit, and registerCard.
In `@src/main.tsx`:
- Around line 16-22: The app currently waits on enableMocking() (which calls
worker.start()) and only renders inside its then callback so a rejected start
blocks rendering; update the boot sequence so
createRoot(...).render(<StrictMode><RouterProvider router={router}
/></StrictMode>) runs regardless of worker.start outcome by calling
enableMocking() in a try/catch (or use .catch/.finally) around worker.start to
log the error and continue, or decouple mocking initialization from rendering
(call enableMocking() without awaiting before invoking createRoot), referencing
enableMocking, worker.start, createRoot, RouterProvider and router.
In `@src/mocks/db.ts`:
- Around line 5-7: cardDB.list currently returns the internal cards array
directly, exposing mutable state and risking test contamination; change
cardDB.list to return a read-only copy of the array (e.g., a shallow copy like
slice/spread or a mapped copy of items) so callers cannot mutate the internal
cards storage; update the implementation referenced by cardDB.list to return the
copied array instead of the original.
In `@src/mocks/handlers.ts`:
- Around line 20-26: The isInvalidExpirationDate function currently parses MM/YY
but ignores the year (it only extracts month and calls
expirationDateValidator(month, 0)), so year-related invalid values pass; update
isInvalidExpirationDate to extract both month and year from the regex (use the
second capture for year) and pass the parsed year into expirationDateValidator
(or convert two-digit YY to a full year if the validator expects a full year) so
both month and year are validated together; refer to isInvalidExpirationDate and
expirationDateValidator when making the change.
In `@src/pages/CardListPage.tsx`:
- Line 60: In the CardListPage component's style block (in CardListPage), fix
the CSS typo by replacing the incorrect property name "font-wieght" with the
correct "font-weight" so the intended bold weight (e.g., 700) is applied; search
for the style rule inside the CardListPage component to locate and update the
misspelled property.
In `@src/utils/cards.ts`:
- Around line 19-22: formatCardNumber always prints two "**** ****" blocks;
change it to compute the middle mask dynamically in formatCardNumber by: take
the existing first = number.slice(0,4) and last = number.slice(-4) (guard for
short inputs), compute maskLen = number.length - first.length - last.length,
create maskedMiddle = '*' repeated maskLen, then format maskedMiddle into
space-separated groups (e.g., groups of 4 from the left) so the output becomes
`${first} ${maskedMiddleFormatted} ${last}`; ensure you handle 14/15/16 lengths
and trim extra spaces for edge cases.
---
Nitpick comments:
In `@src/components/Card/CardList/FabllbackView.tsx`:
- Around line 47-51: HintText currently uses a semantic <strong> element but
should be a neutral text element; change the styled component HintText from
styled.strong to a non-semantic inline/paragraph element (e.g., styled.span or
styled.p) and remove any implicit bold semantics (ensure font-weight remains 400
as in the CSS). Update the component definition named HintText so it renders as
a neutral text node for accessibility and correct semantics.
In `@src/mocks/handlers.ts`:
- Around line 28-31: validateRegisterCard currently only rejects CVC '000',
allowing other invalid CVCs through; update validation to enforce the same CVC
rules used by the form (numeric-only and correct length) instead of the
hardcoded '000' check. Modify validateRegisterCard to call or reuse a shared
validator (or implement a helper) that verifies body.cvc matches /^[0-9]{3,4}$/
(or your project's CVC rule) and return errorResponse(400, 'INVALID_CVC') on
failure; keep existing checks for isInvalidCardNumber and
isInvalidExpirationDate and ensure the function accepts RegisterCardRequest
inputs and uses the same errorResponse semantics.
🪄 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: 0617fa66-f149-485c-86e5-6a1242b0a8ca
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/assets/delete.svgis excluded by!**/*.svgsrc/assets/error.svgis excluded by!**/*.svg
📒 Files selected for processing (39)
.gitignorepackage.jsonpublic/mockServiceWorker.jssrc/App.tsxsrc/__tests__/CardListPage.test.tsxsrc/__tests__/CardRegistrationPage.test.tsxsrc/apis/api.tssrc/apis/cards.tssrc/components/Card/CardList/FabllbackView.tsxsrc/components/Card/CardList/RegisteredCardItem.tsxsrc/components/Card/CardList/RegisteredCardList.tsxsrc/components/Card/CardList/SkeletonCardList.tsxsrc/components/Card/CardPreview/CardNumbers.tsxsrc/components/Card/CardPreview/CardPreview.stories.tsxsrc/components/Card/CardPreview/CardPreview.tsxsrc/components/Common/Form/InputFieldForm.tsxsrc/components/Common/Skeleton/Skeleton.tsxsrc/components/Form/PaymentForm.tsxsrc/components/Layout/CardListLayout.tsxsrc/components/Layout/FormLayout.tsxsrc/components/Select/CardSelect.tsxsrc/constants/cards.tssrc/constants/inputField.tssrc/hooks/useQuery.tssrc/main.tsxsrc/mocks/browser.tssrc/mocks/db.tssrc/mocks/handlers.tssrc/mocks/server.tssrc/mocks/setup.tssrc/pages/CardListPage.tsxsrc/pages/RegistrationCompletionPage.tsxsrc/reset.csssrc/routes/routes.tsxsrc/types/card.tssrc/types/field.tssrc/utils/cards.tssrc/utils/fields.tsvite.config.ts
|
|
||
| it('등록된 카드가 있으면 카드사·번호·유효기간을 렌더링하고 개수를 표시한다', async () => { | ||
| cardDB.add({ | ||
| number: '4111111111111111', |
There was a problem hiding this comment.
테스트 코드의 PAN 형태 하드코딩은 피하는 편이 안전합니다.
Line 29, Line 35 값은 보안 스캐너에서 민감정보로 오탐/차단될 수 있습니다. 테스트 픽스처를 토큰화된 더미 값(또는 마스킹된 fixture 생성 유틸)로 바꿔 스캔 리스크를 줄여 주세요.
Also applies to: 35-35
🧰 Tools
🪛 OpenGrep (1.21.0)
[ERROR] 29-29: Possible credit card number (PAN) detected in source code. Credit card numbers should never be hardcoded or stored in source files. Use a secrets manager or tokenization service instead.
(coderabbit.pii.credit-card-number)
🤖 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/__tests__/CardListPage.test.tsx` at line 29, Replace the hardcoded PAN
strings used in the test fixtures with non-sensitive tokenized or masked values:
find the test card objects in CardListPage.test.tsx that set the number property
(the literal 'number' fields at the two occurrences flagged) and substitute a
tokenized dummy (e.g. a "tok_" token) or a masked PAN (e.g. 4111********1111) or
generate them via a fixture helper (e.g. tokenizePan/maskPan) so tests no longer
contain real PAN-like strings; update both occurrences consistently and ensure
any assertions that depend on the masked/tokenized form are adjusted
accordingly.
| @@ -0,0 +1,54 @@ | |||
| const BASE_URL = import.meta.env.VITE_BASE_URL; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant file
ls -la src/apis || true
sed -n '1,200p' src/apis/api.ts
# Search for VITE_BASE_URL and BASE_URL usage across the repo
rg -n "VITE_BASE_URL|BASE_URL|import\.meta\.env" -S .
# Inspect surrounding lines if file is larger
wc -l src/apis/api.tsRepository: woowacourse/react-payments
Length of output: 2432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "http\.post\(" -S . || true
rg -n "Content-Type" -S src || true
rg -n "VITE_BASE_URL" -S src || trueRepository: woowacourse/react-payments
Length of output: 211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect mock handler for POST
sed -n '1,140p' src/mocks/handlers.ts
# Search for VITE_BASE_URL usage/definition in repo
rg -n "VITE_BASE_URL" -S . || true
# List root env files if present
ls -la . || true
find . -maxdepth 3 -type f \( -name ".env*" -o -name "*env*" \) | head -n 50 || true
# Check type definitions for import.meta.env
rg -n "ImportMetaEnv|import\.meta\.env" -S src || true
# Inspect any config that documents required env vars
find . -maxdepth 2 -type f -iname "README*" -o -iname "env*" -o -iname "vite.config.*" | head -n 50 || trueRepository: woowacourse/react-payments
Length of output: 4418
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' vite.config.ts
sed -n '1,200p' README.md
sed -n '1,120p' src/vite-env.d.ts
# Also check whether BASE_URL typo exists in any config (.env samples)
rg -n "VITE_BASE_URL|BASE_URL" -S .env* vite.config.ts src/README* README.md || true
# Inspect api.ts further for any headers or request wrapper behavior
sed -n '1,220p' src/apis/api.tsRepository: woowacourse/react-payments
Length of output: 6896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "VITE_BASE_URL" -S . || true
rg -n "import\.meta\.env\.BASE_URL|import\.meta\.env\." -S src || true
# Check for any env documentation/samples
rg -n "VITE_|BASE_URL" README.md package.json vite.config.ts index.html tsconfig*.json src -S || true
# List potential env sample files
find . -maxdepth 4 -type f \( -iname ".env*" -o -iname "*env*" \) | sort | head -n 200 || trueRepository: woowacourse/react-payments
Length of output: 1294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,120p' src/main.tsx
sed -n '1,120p' src/constants/cards.ts
sed -n '1,120p' src/vite-env.d.ts
sed -n '1,140p' src/apis/api.tsRepository: woowacourse/react-payments
Length of output: 4031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,200p' vite.config.ts
sed -n '1,200p' index.html
rg -n "BASE_URL|VITE_BASE_URL" vite.config.ts index.html src -SRepository: woowacourse/react-payments
Length of output: 2574
import.meta.env 키 불일치로 인해 API/모킹 경로가 깨질 수 있어요.
src/apis/api.ts는 import.meta.env.VITE_BASE_URL을 쓰지만, src/main.tsx와 src/constants/cards.ts는 import.meta.env.BASE_URL을 읽고 있어요. Vite에서는 보통 VITE_*만 import.meta.env에 노출되므로 BASE_URL이 undefined가 되면 mockServiceWorker.js/카드 아이콘 경로가 깨질 수 있습니다. 또한 api.ts도 VITE_BASE_URL에 대한 값 검증/기본값이 없어 누락 시 ${BASE_URL}${path}가 undefined/cards처럼 만들어질 가능성이 큽니다.
🤖 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/apis/api.ts` at line 1, The BASE_URL constant in src/apis/api.ts uses
import.meta.env.VITE_BASE_URL while other places read import.meta.env.BASE_URL,
which can cause undefined paths; update the module to consistently read the
Vite-exposed variable and provide a safe fallback/validation: resolve the base
value from import.meta.env.VITE_BASE_URL || import.meta.env.BASE_URL || "/" (or
a configured default), validate it is not empty, and ensure any URL-building
logic (e.g., the BASE_URL constant and functions that construct
`${BASE_URL}${path}`) uses this validated value or throws/logs a clear error so
mockServiceWorker and card icon paths don’t become "undefined/…". Ensure other
modules (src/main.tsx and src/constants/cards.ts) reference the same validated
source or an exported getter to keep behavior consistent.
| post: <TResponse, TBody = unknown>(path: string, body: TBody, options?: RequestOptions) => | ||
| request<TResponse>(path, { | ||
| ...options, | ||
| method: 'POST', | ||
| body: JSON.stringify(body), | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant section of src/apis/api.ts
echo "== src/apis/api.ts: lines 1-120 (context) =="
nl -ba src/apis/api.ts | sed -n '1,120p'
echo
echo "== src/apis/api.ts: lines 30-80 (focused) =="
nl -ba src/apis/api.ts | sed -n '30,80p'
# Find request<TResponse> definition
echo
echo "== Searching for request<TResponse> definition/usages =="
rg -n "request<" src/apis/api.ts src/apis -S || true
rg -n "function request|const request|request\s*<" src -S || true
# Find RequestOptions / headers handling
echo
echo "== Searching for RequestOptions and headers merge logic =="
rg -n "type RequestOptions|interface RequestOptions|RequestOptions" src -S || true
rg -n "headers" src/apis/api.ts src/apis -S || true
rg -n "Content-Type|Accept" src -S || trueRepository: woowacourse/react-payments
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify file exists
echo "== File check =="
ls -la src/apis/api.ts || true
echo
echo "== src/apis/api.ts (lines 1-120) =="
cat -n src/apis/api.ts | sed -n '1,120p'
echo
echo "== src/apis/api.ts (lines 30-80) =="
cat -n src/apis/api.ts | sed -n '30,80p'
echo
echo "== Searching for request<TResponse> definition/usages =="
rg -n "request<" src/apis/api.ts src/apis -S || true
rg -n "function request|const request|request\s*<" src -S || true
echo
echo "== Searching for RequestOptions and headers/content-type merge logic =="
rg -n "type RequestOptions|interface RequestOptions|RequestOptions" src -S || true
rg -n "headers" src/apis/api.ts src/apis -S || true
rg -n "Content-Type|Accept" src -S || trueRepository: woowacourse/react-payments
Length of output: 4085
POST JSON 요청 시 기본 Content-Type/Accept를 보장해 주세요.
src/apis/api.ts의http.post는JSON.stringify(body)만 하고Content-Type: application/json(및Accept)을 기본으로 설정/병합하지 않습니다.options.headers가 있어도 기본값이 없어서 서버가application/json을 강제하는 경우 400/415가 날 수 있으니,options.headers와 병합해 기본 헤더를 넣는 방식으로 확인이 필요합니다.
🤖 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/apis/api.ts` around lines 42 - 47, The post helper in api.post currently
JSON.stringify(body) but doesn't ensure Content-Type: application/json and
Accept: application/json; update post to merge default headers into
options.headers when calling request (preserve any user-supplied headers and do
not overwrite them), e.g., set defaults { 'Content-Type': 'application/json',
Accept: 'application/json' } and merge with options.headers before passing the
options object to request<TResponse>, keeping the method: 'POST' and body:
JSON.stringify(body) as-is.
| <IconWrapper>{icon}</IconWrapper> | ||
| <Title>{title}</Title> | ||
| <HintText>{description}</HintText> | ||
| <RegisterButton onClick={action.onClick}>{action.label}</RegisterButton> |
There was a problem hiding this comment.
RegisterButton에 type="button"을 명시해 주세요.
Line 20의 버튼은 재사용 시 폼 내부에서 의도치 않은 submit을 유발할 수 있습니다. FallbackView는 액션 트리거 용도라서 명시적으로 버튼 타입을 고정하는 편이 안전합니다.
Also applies to: 53-61
🤖 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/Card/CardList/FabllbackView.tsx` at line 20, The
RegisterButton in FallbackView is missing an explicit type attribute which can
cause unintended form submissions; update the JSX where RegisterButton is
rendered (the instance using action.onClick and action.label inside
FallbackView) to include type="button" and do the same for the other
RegisterButton occurrences referenced (the block around lines 53-61) so these
action-trigger buttons cannot accidentally submit surrounding forms.
| const issuer = Object.values(CARD_ISSUER_CONFIG).filter( | ||
| (issuer) => issuer.issuerCode === data.issuerCode | ||
| )[0]; | ||
|
|
||
| return ( | ||
| <Wrapper> | ||
| <Issuer $color={issuer.color}></Issuer> | ||
|
|
||
| <Content> | ||
| <Name>{issuer.name}</Name> | ||
| <CardNumber>{formatCardNumber(data.number)}</CardNumber> |
There was a problem hiding this comment.
알 수 없는 issuerCode에 대한 방어 로직이 필요합니다.
Line 13-15에서 매칭 실패 시 issuer가 undefined가 되고, Line 19/22에서 바로 접근하면서 렌더 크래시가 발생합니다. find + null 가드(또는 기본 issuer 값)로 안전하게 처리해 주세요.
🤖 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/Card/CardList/RegisteredCardItem.tsx` around lines 13 - 23,
RegisteredCardItem currently assumes issuer exists which can be undefined;
replace the filter with .find on CARD_ISSUER_CONFIG and add a null-guard or
default issuer object to prevent render crashes. Specifically, update the lookup
(use CARD_ISSUER_CONFIG.find(...) to get issuer), then if issuer is falsy set a
sensible fallback (e.g., { name: 'Unknown', color: '`#ccc`', issuerCode:
data.issuerCode }) or short-circuit render a placeholder; ensure the JSX uses
the safe issuer (Issuer $color, Name, etc.) so accessing issuer.color/name never
throws. Also keep formatCardNumber(data.number) usage unchanged but ensure the
component still renders safely when issuer is missing.
| enableMocking().then(() => { | ||
| createRoot(document.getElementById('root')!).render( | ||
| <StrictMode> | ||
| <RouterProvider router={router} /> | ||
| </StrictMode> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the referenced file with line numbers
sed -n '1,120p' src/main.tsx | cat -n
# Find where enableMocking is defined/implemented and inspect it
rg -n "enableMocking" -S .Repository: woowacourse/react-payments
Length of output: 907
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the mocking entry file
sed -n '1,200p' src/mocks/browser.ts | cat -n
# Also search for any error handling around worker.start usage
rg -n "worker\.start\(" -S srcRepository: woowacourse/react-payments
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "createRoot\\(" -S .
rg -n "RouterProvider" -S srcRepository: woowacourse/react-payments
Length of output: 266
🌐 Web query:
MSW setupWorker worker.start() promise reject on failure
💡 Result:
In MSW (browser), worker.start returns a Promise that resolves when the Service Worker is ready; you should await it because worker registration/activation is asynchronous. [1] The Promise can reject if MSW fails during the start flow (e.g., Service Worker registration/activation/script integrity checks). MSW’s internal implementation treats start as an async chain (startWorkerInstance.then(...); any errors in that chain propagate to the returned Promise). [2] Recommended error handling pattern: const worker = setupWorker(...handlers) worker.start({ quiet: true }).catch((err) => { // start failed: worker did not become ready to mock console.error('MSW worker failed to start', err) throw err // optionally fail fast }) // or with async/await try { await worker.start({ quiet: true }) } catch (err) { console.error('MSW worker failed to start', err) throw err } Notes: - If you don’t await worker.start, you can hit race conditions where your app makes requests before mocking is enabled, even though start itself resolves “when the worker is ready” per MSW docs. [1] - Also ensure you only call setupWorker once per page; creating multiple workers can lead to requests not being intercepted as expected (which can look like “start failed”). [3] If you share your framework (CRA/Next/Vite) and the exact console error/stack trace, I can map it to the specific MSW start-stage that’s rejecting.
Citations:
- 1: https://mswjs.io/docs/api/setup-worker/start/
- 2: https://github.com/mswjs/msw/blob/main/src/browser/setupWorker/start/createStartHandler.ts
- 3: MSW is not handling requests after
setupWorkeris called more than once mswjs/msw#2585
MSW 워커 시작 실패 시 앱 렌더가 실행되지 않습니다.
src/main.tsx에서 enableMocking()(내부적으로 worker.start(...))을 then만으로 연결하고 catch가 없어, worker.start()가 reject되면 createRoot(...).render(...)가 호출되지 않습니다(빈 화면 가능). try/catch로 start 실패를 처리하거나, 실패 여부와 무관하게 최소 렌더가 진행되도록 부팅 흐름을 분리하세요.
🤖 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/main.tsx` around lines 16 - 22, The app currently waits on
enableMocking() (which calls worker.start()) and only renders inside its then
callback so a rejected start blocks rendering; update the boot sequence so
createRoot(...).render(<StrictMode><RouterProvider router={router}
/></StrictMode>) runs regardless of worker.start outcome by calling
enableMocking() in a try/catch (or use .catch/.finally) around worker.start to
log the error and continue, or decouple mocking initialization from rendering
(call enableMocking() without awaiting before invoking createRoot), referencing
enableMocking, worker.start, createRoot, RouterProvider and router.
| export const cardDB = { | ||
| list: (): Card[] => cards, | ||
|
|
There was a problem hiding this comment.
내부 상태 배열이 직접 노출되어 테스트 간 오염 위험이 있습니다.
Line 6의 list()가 원본 배열을 그대로 반환해서, 호출 측의 실수로 mock DB 상태가 바뀔 수 있습니다. 읽기 전용 복사본 반환(또는 불변 뷰)로 캡슐화를 유지하는 방향이 더 안전합니다.
🤖 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/mocks/db.ts` around lines 5 - 7, cardDB.list currently returns the
internal cards array directly, exposing mutable state and risking test
contamination; change cardDB.list to return a read-only copy of the array (e.g.,
a shallow copy like slice/spread or a mapped copy of items) so callers cannot
mutate the internal cards storage; update the implementation referenced by
cardDB.list to return the copied array instead of the original.
| const isInvalidExpirationDate = (expirationDate: string) => { | ||
| const match = expirationDate.match(/^(\d{2})\/(\d{2})$/); | ||
| if (!match) return true; | ||
|
|
||
| const [, month] = match; | ||
| return expirationDateValidator(month, 0).error; | ||
| }; |
There was a problem hiding this comment.
만료일 검증에서 연도가 사실상 무시되고 있습니다.
MM/YY를 파싱해도 Line 25에서 year를 쓰지 않아, 연도 관련 잘못된 값이 검증을 통과할 수 있습니다. 의도적으로 연도 검증을 제외한 게 아니라면, month/year를 함께 검증하도록 입력 계약을 맞춰 주세요.
🤖 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/mocks/handlers.ts` around lines 20 - 26, The isInvalidExpirationDate
function currently parses MM/YY but ignores the year (it only extracts month and
calls expirationDateValidator(month, 0)), so year-related invalid values pass;
update isInvalidExpirationDate to extract both month and year from the regex
(use the second capture for year) and pass the parsed year into
expirationDateValidator (or convert two-digit YY to a full year if the validator
expects a full year) so both month and year are validated together; refer to
isInvalidExpirationDate and expirationDateValidator when making the change.
| width: 100%; | ||
| margin-bottom: 16px; | ||
| font-size: 18px; | ||
| font-wieght: 700; |
There was a problem hiding this comment.
font-weight 오타로 스타일이 적용되지 않습니다.
Line 60의 font-wieght를 font-weight로 수정해 주세요.
🤖 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/pages/CardListPage.tsx` at line 60, In the CardListPage component's style
block (in CardListPage), fix the CSS typo by replacing the incorrect property
name "font-wieght" with the correct "font-weight" so the intended bold weight
(e.g., 700) is applied; search for the style rule inside the CardListPage
component to locate and update the misspelled property.
| export const formatCardNumber = (number: string): string => { | ||
| const first = number.slice(0, 4); | ||
| const last = number.slice(-4); | ||
| return `${first} **** **** ${last}`; |
There was a problem hiding this comment.
카드 길이(14/15/16)에 맞춰 마스킹 길이를 동적으로 계산해 주세요.
현재는 항상 **** ****를 고정 출력해서 Diners/AMEX도 16자리처럼 보입니다. number.length 기준으로 중간 마스킹 길이를 계산하면 브랜드별 자릿수와 UI가 일치해요. 우선 14/15/16 입력 케이스를 각각 어떻게 보여줄지부터 정해보면 어떨까요?
🤖 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/cards.ts` around lines 19 - 22, formatCardNumber always prints two
"**** ****" blocks; change it to compute the middle mask dynamically in
formatCardNumber by: take the existing first = number.slice(0,4) and last =
number.slice(-4) (guard for short inputs), compute maskLen = number.length -
first.length - last.length, create maskedMiddle = '*' repeated maskLen, then
format maskedMiddle into space-separated groups (e.g., groups of 4 from the
left) so the output becomes `${first} ${maskedMiddleFormatted} ${last}`; ensure
you handle 14/15/16 lengths and trim extra spaces for edge cases.
- queryFn을 deps에서 제거해 인라인 함수 주입 시 무한 요청 방지 - 성공 시 setError(null) 호출로 이전 에러 상태 초기화
2 participants
🎯 페이먼츠
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
1단계
2단계
3단계
idle | loading | success | error네 가지로 명시적으로 관리한다.🕵️ 셀프 리뷰(Self-Review)
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
안녕하세요 데구리! step2가 끝난지 얼마 안된 시점에 다시 뵙네요!
step3 잘 부탁드립니다 🙇🏻♂️
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
step2에 이어 step3에서도 물리적인 시간이 부족했기 때문에, 클린 코드 관점의 리팩토링보다는 step3의 새로운 기능 추가에 집중했습니다. 그중에서도 가장 공을 들인 부분은 step2에서 다루었던 "기능 명세"와 "API 관련 모듈" 입니다.
아래에서 다시 언급하겠지만, 이번에는 스프레드시트를 활용해 기능 명세를 작성해 보았습니다. 구현이 완료된 부분은 녹색으로 표시했고, 아직 구현되지 않은 부분은 흰 바탕으로 남겨 두었습니다.
step3 리뷰를 받은 후에는 그동안 미진했던 리팩토링과 미구현 사항에 집중해 마무리해 나갈 계획입니다!
2) 이번 리뷰를 통해 논의하고 싶은 부분
기능 명세
before
after
기능 명세에 대한 피드백을 반영하여 depth를 도메인 → 기능 단위 → 세부 동작으로 세분화하고, 하나의 유저 스토리에서 여러 피처 리스트가 도출될 수 있도록 구성했습니다. 익숙하지 않은 형식이라 처음 기준을 잡는 데 어려움이 있었지만, AI의 도움을 받아 하나씩 채워나갔습니다.
작성하면서 가장 고민했던 부분은 1 depth를 페이지 기준으로 나눌 것인지, 도메인 기준으로 나눌 것인지였습니다. 페이지들이 유기적으로 연결되어 있어 도메인 기준이 적절하다고 판단했지만, 막상 분류해보니 거의 모든 코드가 '카드' 도메인과 맞닿아 있어 의미 있는 구분이 되는지 의문이 들었습니다.
개발을 하면서 '도메인'이라는 용어를 자주 접하긴 했지만 구체적으로 정의 내려본 적은 없었는데, 이번 명세를 작성하며 나름대로 '관련된 것들을 같은 관심사로 묶어주는 상위 개념' 정도로 이해해 보았습니다. 미션 규모가 작아서 구분이 모호하게 느껴진 것일 수도 있을 텐데, 현업에서는 '도메인'을 어떻게 정의하고, 기능 명세를 작성할 때 어떤 기준으로 분류하는지 궁금합니다.
추가로, 이 기능 명세 폼에 대해 조언해주실 부분이나 개선하면 좋을 점이 있다면 말씀해 주시면 감사하겠습니다!
api 인스턴스
src/apis/api.ts에 api 인스턴스를 정의해 보았습니다. 인스턴스를 객체 리터럴(http) 로 구성하고, request 내부 함수에 공통 로직(BASE_URL prefix, 응답 파싱, 에러 처리)을 모아둔 뒤 메서드별로 얇게 래핑하는 구조로 만들었습니다.설계나 타입 활용 측면에서 특히 의견을 듣고 싶은 부분은 다음과 같습니다.
api 인스턴스를 객체 리터럴로 정의해 보았는데, 그동안은 axios 인스턴스를 주로 사용해왔기에 HTTP 클라이언트를 다양하게 설계해 본 경험이 많지 않아 이 방식이 적절한 선택이었는지 고민이 됐던 것 같습니다.
현재는 인스턴스가 하나뿐이고 내부 상태도 없어 객체 리터럴로 충분하다고 판단했지만, 추후 baseURL이나 헤더가 다른 인스턴스를 여러 개 다뤄야 할 경우를 떠올리니 createHttpClient(config) 형태의 팩토리 함수가 더 적합하지 않을까 하는 생각도 들었습니다.
현업에서는 보통 어떤 형태로 HTTP 클라이언트를 구성하는지, 참고할 만한 패턴이나 코드 예시가 있다면 함께 알려주시면 감사하겠습니다!
지금은 !res.ok일 때 throw new Error(HTTP ${res.status}) 형태로 단순히 상태 코드 문자열만 던지고 있는데, 이렇게 두면 호출부에서 상태 코드별 분기(401 → 로그인 페이지, 404 → 빈 상태 등)나 서버가 내려준 에러 메시지 활용이 어렵겠다는 생각이 들었습니다.
호출부에서 다루기 좋고 확장에도 유연한 에러 처리 구조가 있다면 추천해 주실 수 있을지 궁금합니다.
res.text() 후 분기해 JSON.parse를 호출하고 있는데, JSON이 아닌 응답(예: 204 statusCode의 빈 스트링 '')이 섞일 수 있는 환경이라 이렇게 작성했습니다. res.json()을 바로 쓰는 방식과 비교해 어느 쪽이 더 일반적인지 궁금합니다.
useQuery
해당 요구사항을 만족하기 위해 고민하던 중, tanstack-query의 useQuery와 유사한 형태의 커스텀 훅을 만드는 방향이 적절하겠다고 판단했습니다. 비동기 상태는 status라는 단일 상태로 관리하고, 패칭 흐름의 각 시점에 맞춰 값을 변경하는 방식으로 구현했습니다.
또한 카드 제거 후 목록을 최신화하기 위한 리패칭 로직도 필요했는데, 훅 내부에 refetchKey라는 트리거용 상태를 두고 호출부에서 refetch를 호출하면 이 값을 증가시켜 useEffect의 의존성 배열을 통해 재실행되도록 구성했습니다.
설계하면서 특히 의견을 듣고 싶은 부분은 다음과 같습니다.
전반적인 설계 방향이 적절한지 궁금합니다. status 단일 상태에서 isLoading, isError 등을 파생시키는 구조나 반환 인터페이스가 호출부에서 다루기에 자연스러운지 봐주시면 감사하겠습니다.
refetchKey를 트리거로 사용하는 리패칭 방식입니다. 카운터를 증가시켜 effect를 다시 돌리는 패턴이 직관적이긴 하지만 다소 우회적인 느낌도 있어, 더 일반적이거나 깔끔한 방식이 있을지 의견을 듣고 싶습니다.
cancelled 플래그를 통한 race condition 방어가 충분한지, 그리고 그 외에 전반적으로 더 개선하면 좋을 부분이 있다면 알려주시면 감사하겠습니다!
통합 테스트
React Testing Library를 제대로 활용해 보고 싶은 마음이 있었지만, 우선순위에서 밀려 결국 AI의 도움을 받아 테스트 코드를 작성하게 되었습니다. 미션을 진행할 때마다 테스트 코드가 가장 후순위로 밀리는 경우가 많은데, 그렇다 보니 TDD가 아닌 상황에서 테스트 코드를 어느 시점에 작성하는 것이 좋은지가 여전히 어렵게 느껴집니다.
또한 '통합 테스트'라는 개념도 아직 명확히 와닿지 않습니다. 어느 정도 범위까지를 통합 테스트로 보아야 하는지, 그리고 데구리만의 테스트 코드 작성 요령이나 기준이 있다면 공유해 주시면 감사하겠습니다!
리뷰에 시간 내주셔서 감사합니다!
✅ 리뷰어 체크 포인트
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*를 사용했는가?