Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Walkthrough온보딩 플로우를 훅 기반으로 리팩터링했습니다. URL 쿼리 기반의 Changes
Sequence Diagram(s)(생성 기준 미충족 — 건너뜀) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
|
✅ Storybook chromatic 배포 확인: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx (1)
168-172:⚠️ Potential issue | 🟡 MinorMac 스텝 스킵 시 GA 이벤트 누락
shouldSkipMacStep분기에서setStep(Step.FINAL)은 호출하지만sendGAEvent는 호출하지 않습니다. 정상 흐름(lines 189-193)과 달리 GA 이벤트가 누락되어 스텝 전환 지표에 갭이 생깁니다. 의도적으로 제외한 것이라면 주석으로 명시해 주세요.📊 GA 이벤트 추가 예시
if (shouldSkipMacStep) { setDirection(1); setStep(Step.FINAL); + sendGAEvent( + `onboard-step-${currentIndex + 1}`, + `onboard-step-${currentIndex + 1}`, + `onboard-step-${currentIndex + 1}` + ); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx` around lines 168 - 172, When handling the shouldSkipMacStep branch in MainCard.tsx you setDirection(1) and setStep(Step.FINAL) but do not call sendGAEvent, causing a missing GA step-change event; update the branch to invoke sendGAEvent with the same event payload used in the normal flow (the one used after successful step changes) immediately before or after setStep(Step.FINAL), or if omission was intentional add a clear comment explaining why GA should not be sent for shouldSkipMacStep so reviewers know it’s deliberate; reference the shouldSkipMacStep conditional, setDirection, setStep, Step.FINAL, and sendGAEvent when implementing the change.
🧹 Nitpick comments (4)
apps/client/src/shared/hooks/useFunnel.ts (2)
30-38:setStep기본값replace = true의 의도를 명시할 것
setStep의 기본값이replace = true이므로goNext()/goPrev()를 옵션 없이 호출하면 항상 히스토리 엔트리를 교체합니다. 결과적으로 브라우저 뒤로가기 버튼으로 funnel 단계를 거슬러 올라갈 수 없습니다. 온보딩처럼 뒤로가기를 막아야 하는 흐름에서는 의도적인 선택이지만, 범용 shared 훅으로 공개된 만큼 기본 동작을 주석이나 JSDoc으로 명시하는 것을 권장합니다.📝 JSDoc 문서화 예시
+ /** + * 지정한 스텝으로 이동합니다. + * `@param` replace - 기본값 true: 히스토리를 교체(push 없음). false로 지정하면 새 히스토리 엔트리가 쌓입니다. + */ const setStep = useCallback( (nextStep: TStep, { replace = true }: MoveStepOptions = {}) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/shared/hooks/useFunnel.ts` around lines 30 - 38, The setStep function currently defaults its MoveStepOptions parameter to { replace = true }, which causes setSearchParams to replace the current history entry (so goNext/goPrev without options won't create back/forward entries); add a brief JSDoc comment above setStep (and mention behavior in any exported hook-level JSDoc) that documents the default replace=true behavior, its effect on browser history and back-button navigation, and note how callers can override it by passing { replace: false } to create new history entries; reference the setStep function and the goNext/goPrev helpers so reviewers can find and understand the intended default and how to opt out.
28-28:currentIndex를useMemo로 메모화 권장
currentStep은useMemo로 안정화되어 있지만currentIndex는 매 렌더마다steps.indexOf()를 재실행합니다. 스텝 수가 적어 실질적 영향은 작지만,goNext/goPrev의useCallback의존 배열에도 포함되므로 일관성을 위해 메모화하는 것이 좋습니다.♻️ useMemo 적용 예시
- const currentIndex = steps.indexOf(currentStep); + const currentIndex = useMemo( + () => steps.indexOf(currentStep), + [steps, currentStep] + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/shared/hooks/useFunnel.ts` at line 28, The calculation of currentIndex in the useFunnel hook should be memoized: replace the direct const currentIndex = steps.indexOf(currentStep) with a useMemo that computes steps.indexOf(currentStep) and depends on [steps, currentStep] so currentIndex is stable across renders and consistent for the goNext/goPrev useCallback dependency arrays; update references to currentIndex unchanged.apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx (2)
197-202: 훅에서 제공하는isFirstStep을 활용하는 것이 일관적
currentIndex > 0조건은useFunnel이 이미 반환하는isFirstStep의 역(inverse)입니다. 훅의 추상화를 활용하면 의도가 더 명확해집니다.♻️ isFirstStep 활용 예시
- const { currentStep: step, currentIndex, setStep, goNext, goPrev } = + const { currentStep: step, currentIndex, isFirstStep, setStep, goNext, goPrev } = useFunnel<StepType>({ steps: stepOrder, initialStep: Step.STORY_0, }); ... const prevStep = () => { - if (currentIndex > 0) { + if (!isFirstStep) { setDirection(-1); goPrev(); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx` around lines 197 - 202, Replace the manual check in prevStep with the hook-provided flag: use isFirstStep from useFunnel instead of comparing currentIndex > 0; update the prevStep function (which calls setDirection and goPrev) to guard with if (!isFirstStep) { setDirection(-1); goPrev(); } so the intent relies on the hook abstraction and remains consistent with other navigation logic.
152-157:stepOrder를 직접 참조하는 peek 패턴이 훅 추상화를 우회함
stepOrder[currentIndex + 1]로 다음 스텝을 미리 확인하는 로직이useFunnel에 전달된steps가 아닌 모듈 상수stepOrder에 직접 의존합니다. 현재는 동일한 배열이므로 문제없지만, 향후 두 배열이 달라지면 스킵 로직만 조용히 깨질 수 있습니다.훅에
peekNext()유틸을 추가하거나, 아래처럼 훅이 반환하는 값을 활용하는 것을 고려해보세요.♻️ useFunnel에 peekNext 추가 예시 (useFunnel.ts)
+ const peekNext = useCallback( + () => steps[currentIndex + 1] ?? null, + [steps, currentIndex] + ); + return { currentStep, currentIndex, isFirstStep: currentIndex <= 0, isLastStep: currentIndex === steps.length - 1, setStep, goNext, goPrev, + peekNext, };그리고 MainCard.tsx에서:
- const next = stepOrder[currentIndex + 1]; + const next = peekNext();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx` around lines 152 - 157, The nextStep function currently peeks the next step by directly reading the module-level stepOrder (stepOrder[currentIndex + 1]), which bypasses the useFunnel abstraction; update the code so MainCard does not reference stepOrder directly: add a peekNext (or getNext) utility to useFunnel that returns the next step based on the hook's internal steps/currentIndex, then replace uses of stepOrder[currentIndex + 1] in nextStep with that hook-provided value and compute isMacStep/shouldSkipMacStep from the returned next step (keep existing symbols: nextStep, currentIndex, isMacStep, shouldSkipMacStep, useFunnel, steps).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/client/src/shared/hooks/useFunnel.ts`:
- Around line 14-26: The hook useFunnel fails to validate that the provided
initialStep exists in steps which can cause currentIndex === -1 and break
functions like goNext; add a runtime guard inside useFunnel that checks if
initialStep is included in steps (use Array.prototype.includes on steps) and if
not either (a) in production: fall back to a safe default (e.g., steps[0]) and
log a clear error, or (b) in development: throw or assert to surface the
misconfiguration; update uses of currentStep/currentIndex/goNext to rely on the
validated value (e.g., initialStepValidated) so you never compute an index from
an invalid initialStep.
---
Outside diff comments:
In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx`:
- Around line 168-172: When handling the shouldSkipMacStep branch in
MainCard.tsx you setDirection(1) and setStep(Step.FINAL) but do not call
sendGAEvent, causing a missing GA step-change event; update the branch to invoke
sendGAEvent with the same event payload used in the normal flow (the one used
after successful step changes) immediately before or after setStep(Step.FINAL),
or if omission was intentional add a clear comment explaining why GA should not
be sent for shouldSkipMacStep so reviewers know it’s deliberate; reference the
shouldSkipMacStep conditional, setDirection, setStep, Step.FINAL, and
sendGAEvent when implementing the change.
---
Nitpick comments:
In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx`:
- Around line 197-202: Replace the manual check in prevStep with the
hook-provided flag: use isFirstStep from useFunnel instead of comparing
currentIndex > 0; update the prevStep function (which calls setDirection and
goPrev) to guard with if (!isFirstStep) { setDirection(-1); goPrev(); } so the
intent relies on the hook abstraction and remains consistent with other
navigation logic.
- Around line 152-157: The nextStep function currently peeks the next step by
directly reading the module-level stepOrder (stepOrder[currentIndex + 1]), which
bypasses the useFunnel abstraction; update the code so MainCard does not
reference stepOrder directly: add a peekNext (or getNext) utility to useFunnel
that returns the next step based on the hook's internal steps/currentIndex, then
replace uses of stepOrder[currentIndex + 1] in nextStep with that hook-provided
value and compute isMacStep/shouldSkipMacStep from the returned next step (keep
existing symbols: nextStep, currentIndex, isMacStep, shouldSkipMacStep,
useFunnel, steps).
In `@apps/client/src/shared/hooks/useFunnel.ts`:
- Around line 30-38: The setStep function currently defaults its MoveStepOptions
parameter to { replace = true }, which causes setSearchParams to replace the
current history entry (so goNext/goPrev without options won't create
back/forward entries); add a brief JSDoc comment above setStep (and mention
behavior in any exported hook-level JSDoc) that documents the default
replace=true behavior, its effect on browser history and back-button navigation,
and note how callers can override it by passing { replace: false } to create new
history entries; reference the setStep function and the goNext/goPrev helpers so
reviewers can find and understand the intended default and how to opt out.
- Line 28: The calculation of currentIndex in the useFunnel hook should be
memoized: replace the direct const currentIndex = steps.indexOf(currentStep)
with a useMemo that computes steps.indexOf(currentStep) and depends on [steps,
currentStep] so currentIndex is stable across renders and consistent for the
goNext/goPrev useCallback dependency arrays; update references to currentIndex
unchanged.
Comment on lines
+14
to
+26
| export function useFunnel<TStep extends string>({ | ||
| steps, | ||
| initialStep, | ||
| queryKey = 'step', | ||
| }: UseFunnelOptions<TStep>) { | ||
| const [searchParams, setSearchParams] = useSearchParams(); | ||
|
|
||
| const currentStep = useMemo(() => { | ||
| const value = searchParams.get(queryKey); | ||
| return value && steps.includes(value as TStep) | ||
| ? (value as TStep) | ||
| : initialStep; | ||
| }, [searchParams, queryKey, steps, initialStep]); |
There was a problem hiding this comment.
initialStep이 steps 배열에 포함되지 않을 경우 조용히 오동작
런타임에 initialStep이 steps에 포함되어 있는지 검증하지 않습니다. 잘못 구성된 경우 currentIndex === -1이 되어 goNext()가 steps[0]으로 이동하는 등 예측 불가한 동작이 발생합니다.
🛡️ 개발 환경용 런타임 가드 예시
export function useFunnel<TStep extends string>({
steps,
initialStep,
queryKey = 'step',
}: UseFunnelOptions<TStep>) {
+ if (process.env.NODE_ENV !== 'production' && !steps.includes(initialStep)) {
+ throw new Error(
+ `[useFunnel] initialStep "${initialStep}" is not in the steps array.`
+ );
+ }
+
const [searchParams, setSearchParams] = useSearchParams();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/client/src/shared/hooks/useFunnel.ts` around lines 14 - 26, The hook
useFunnel fails to validate that the provided initialStep exists in steps which
can cause currentIndex === -1 and break functions like goNext; add a runtime
guard inside useFunnel that checks if initialStep is included in steps (use
Array.prototype.includes on steps) and if not either (a) in production: fall
back to a safe default (e.g., steps[0]) and log a clear error, or (b) in
development: throw or assert to surface the misconfiguration; update uses of
currentStep/currentIndex/goNext to rely on the validated value (e.g.,
initialStepValidated) so you never compute an index from an invalid initialStep.
jjangminii
approved these changes
Feb 21, 2026
Collaborator
jjangminii
left a comment
jjangminii
left a comment
There was a problem hiding this comment.
mainCard 코드가 여럿 거치면서 복잡해졌었는데 퍼널 분리하면 서 깔끔해진 것 같아요 고생하셨습니다-! 궁금한거 하나 달아뒀어요 확인해주세요
| return { | ||
| currentStep, | ||
| currentIndex, | ||
| isFirstStep: currentIndex <= 0, |
Collaborator
There was a problem hiding this comment.
isFirstStep의 currentIndex가 ===0이 아니라 <= 0로 한 이유가 뭔가요?
Member
Author
There was a problem hiding this comment.
해당 로직은 음수의 index값이 나왔을 때에도 첫 번째 step으로 보도록 하는 방어적 로직에 가까워요.
물론 해당 로직은 첫 페이지로 강제 변환을 하지는 않고, isFirstStep에 대한 값만 조정하기는 합니다. 다만 현재 코드 상으로는 쿼리 step이 잘못되면 initialStep으로 대체하므로, initialStep만 정상이라면 -1와 같은 음수 값이 애초에 안나올 것 같기는 합니다!
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts (2)
119-125:goNext()의 반환값을 무시하고 있어 마지막 단계에서 GA 이벤트가 잘못 발송될 수 있습니다.
goNext()는 다음 단계가 없으면null을 반환합니다. 현재 코드는 반환값을 확인하지 않고 항상sendGAEvent를 호출하므로, 만약goNext가 실패(null 반환)하더라도 GA 이벤트가 발송됩니다. 현재 흐름에서는 FINAL 단계가 위에서 처리되어 도달하기 어렵지만, 방어적 코드가 권장됩니다.♻️ goNext 반환값 확인 제안
setDirection(1); - goNext(); - sendGAEvent( - `onboard-step-${currentIndex + 1}`, - `onboard-step-${currentIndex + 1}`, - `onboard-step-${currentIndex + 1}` - ); + const moved = goNext(); + if (moved) { + sendGAEvent( + `onboard-step-${currentIndex + 1}`, + `onboard-step-${currentIndex + 1}`, + `onboard-step-${currentIndex + 1}` + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts` around lines 119 - 125, goNext() can return null when there is no next step, but the code always calls sendGAEvent; change the flow in the block containing setDirection(1); goNext(); sendGAEvent(...) to capture the return of goNext() (e.g., const next = goNext()) and only call sendGAEvent when next is non-null (or truthy). Keep setDirection(1) as-is if desired, but guard sendGAEvent with the goNext() result to prevent firing GA for a non-existent step; reference functions: goNext(), sendGAEvent and the currentIndex usage for event naming.
66-81:useEffect내 비동기 IIFE에 cleanup이 없어 언마운트 후 상태 업데이트가 발생할 수 있습니다.컴포넌트가 언마운트된 후
requestFCMToken이 완료되면setFcmToken,localStorage.setItem,alert등이 호출됩니다. React 19 Strict Mode에서는 이 effect가 두 번 실행되어 중복 alert이 표시될 수도 있습니다.또한 mac 감지(동기)와 FCM 토큰 요청(비동기)이 하나의 effect에 혼재되어 있어 관심사 분리가 되지 않습니다.
♻️ effect 분리 및 cleanup 추가 제안
+ useEffect(() => { + const ua = navigator.userAgent.toLowerCase(); + if (ua.includes('mac os') || ua.includes('iphone') || ua.includes('ipad')) { + setIsMac(true); + } + }, []); - useEffect(() => { - const ua = navigator.userAgent.toLowerCase(); - if (ua.includes('mac os') || ua.includes('iphone') || ua.includes('ipad')) { - setIsMac(true); - } - - (async () => { - const token = await requestFCMToken(); - if (token) { - setFcmToken(token); - localStorage.setItem('FcmToken', token); - } else { - alert('푸시 알람 설정 에러'); - } - })(); - }, [requestFCMToken]); + useEffect(() => { + let cancelled = false; + (async () => { + const token = await requestFCMToken(); + if (cancelled) return; + if (token) { + setFcmToken(token); + localStorage.setItem('FcmToken', token); + } else { + alert('푸시 알람 설정 에러'); + } + })(); + return () => { cancelled = true; }; + }, [requestFCMToken]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts` around lines 66 - 81, The effect mixes sync OS detection with an async IIFE that calls requestFCMToken and updates state (setFcmToken), localStorage ('FcmToken'), and may alert, which can run after unmount and run twice in Strict Mode; split into two effects (one synchronous effect to setIsMac using navigator.userAgent, and a separate effect for the async token flow) and add a cleanup guard in the async effect (e.g., an isMounted flag or AbortController) to prevent calling setFcmToken, localStorage.setItem, or alert after unmount or when the effect is re-run.apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx (1)
129-139: FINAL 단계에서 "다음" 버튼 레이블이 "다음"으로 표시됩니다.마지막 단계(
Step.FINAL)에서도 버튼 텍스트가 "다음"으로 표시되는데, 이 단계에서는 실제로 회원가입 제출이 수행됩니다. 사용자 관점에서 "완료" 또는 "시작하기" 등으로 변경하면 기대 행동과 일치합니다.✏️ 최종 단계 버튼 텍스트 변경 제안
<Button variant="primary" size="medium" className="ml-auto w-[4.8rem]" onClick={nextStep} isDisabled={step === Step.JOB && !jobShareAgree} > - 다음 + {step === Step.FINAL ? '완료' : '다음'} </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx` around lines 129 - 139, 현재 MainCard.tsx의 Button (component: Button, props: onClick={nextStep}, isDisabled tied to jobShareAgree) 항상 "다음" 레이블을 사용하고 있어 Step.FINAL에서의 제출 동작과 맞지 않습니다; 수정하려면 렌더링 로직에서 step === Step.FINAL일 때 버튼 텍스트를 "완료" 또는 "시작하기"로 바꾸고 그 외 단계에서는 기존 "다음"을 유지하도록 조건부 텍스트를 적용하세요 (참조: Step.FINAL, Button, nextStep, jobShareAgree).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts`:
- Around line 38-64: requestFCMToken currently calls registerServiceWorker()
without awaiting it causing a race with getToken; change requestFCMToken to
await service worker registration (or accept a ServiceWorkerRegistration
parameter) and pass that registration to getToken (use the messaging
registration option) so token retrieval happens after registration completes;
also modify the useEffect that triggers requestFCMToken on mount to avoid
prompting for notification permission immediately (invoke requestFCMToken only
in response to a user action or after explicit consent) and update any callers
to provide the registration if you switched to the parameter approach; refer to
requestFCMToken, registerServiceWorker, and the mounting useEffect when making
these changes.
- Around line 105-117: The onError block in the postSignData call inside
useOnboardingFunnel (the isFinalStep branch) incorrectly treats failures as
success by redirecting when a savedEmail exists; change onError to surface the
failure instead of silently redirecting: remove the conditional redirect,
capture the error from the postSignData onError callback and set a visible error
state or call the existing UI notification/toast utility (or expose setError via
the hook) so the UI can show an error message and let the user retry; keep the
successful onSuccess redirect intact and ensure fcmToken null handling remains
unchanged.
- Around line 90-96: When handling alarmSelected in useOnboardingFunnel, avoid
calling normalizeTime on an empty AlarmsType entry: check the computed raw =
AlarmsType[alarmSelected - 1].time for falsy/empty string before calling
normalizeTime; if raw is empty, do not call normalizeTime or set remindTime to a
safe default (e.g., '09:00') or trigger validation so you don't set remindTime
to 'NaN:NaN'; update the branch around isAlarmStep / alarmSelected to guard raw,
call normalizeTime(raw) only when raw is non-empty, and use setRemindTime with
the fallback or validation flow otherwise.
---
Nitpick comments:
In `@apps/client/src/pages/onBoarding/components/funnel/MainCard.tsx`:
- Around line 129-139: 현재 MainCard.tsx의 Button (component: Button, props:
onClick={nextStep}, isDisabled tied to jobShareAgree) 항상 "다음" 레이블을 사용하고 있어
Step.FINAL에서의 제출 동작과 맞지 않습니다; 수정하려면 렌더링 로직에서 step === Step.FINAL일 때 버튼 텍스트를 "완료"
또는 "시작하기"로 바꾸고 그 외 단계에서는 기존 "다음"을 유지하도록 조건부 텍스트를 적용하세요 (참조: Step.FINAL, Button,
nextStep, jobShareAgree).
In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts`:
- Around line 119-125: goNext() can return null when there is no next step, but
the code always calls sendGAEvent; change the flow in the block containing
setDirection(1); goNext(); sendGAEvent(...) to capture the return of goNext()
(e.g., const next = goNext()) and only call sendGAEvent when next is non-null
(or truthy). Keep setDirection(1) as-is if desired, but guard sendGAEvent with
the goNext() result to prevent firing GA for a non-existent step; reference
functions: goNext(), sendGAEvent and the currentIndex usage for event naming.
- Around line 66-81: The effect mixes sync OS detection with an async IIFE that
calls requestFCMToken and updates state (setFcmToken), localStorage
('FcmToken'), and may alert, which can run after unmount and run twice in Strict
Mode; split into two effects (one synchronous effect to setIsMac using
navigator.userAgent, and a separate effect for the async token flow) and add a
cleanup guard in the async effect (e.g., an isMounted flag or AbortController)
to prevent calling setFcmToken, localStorage.setItem, or alert after unmount or
when the effect is re-run.
Comment on lines
+38
to
+64
| const requestFCMToken = useCallback(async (): Promise<string | null> => { | ||
| try { | ||
| const app = getApps().length > 0 ? getApp() : initializeApp(firebaseConfig); | ||
| const messaging = getMessaging(app); | ||
| const permission = await Notification.requestPermission(); | ||
| registerServiceWorker(); | ||
|
|
||
| if (permission !== 'granted') { | ||
| alert('알림 권한 허용이 필요합니다!'); | ||
| return null; | ||
| } | ||
|
|
||
| const forFcmtoken = await getToken(messaging, { | ||
| vapidKey: import.meta.env.VITE_FIREBASE_VAPID_KEY, | ||
| }); | ||
|
|
||
| if (forFcmtoken) { | ||
| return forFcmtoken; | ||
| } | ||
|
|
||
| alert('토큰 생성 실패. 다시 시도해주세요.'); | ||
| return null; | ||
| } catch { | ||
| alert('알림 설정 중 오류가 발생했습니다. 다시 시도해주세요.'); | ||
| return null; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
registerServiceWorker()가 await 없이 호출되어 getToken과 경쟁 조건 발생 가능.
Line 43에서 registerServiceWorker()는 fire-and-forget으로 호출되지만, Line 50의 getToken은 서비스 워커가 등록 완료된 상태를 기대합니다. 서비스 워커 등록이 완료되기 전에 getToken이 호출되면 토큰 생성이 실패할 수 있습니다.
또한 Line 66-80의 useEffect에서 컴포넌트 마운트 즉시 FCM 토큰을 요청하면서 알림 권한 프롬프트가 사용자 의도 없이 표시됩니다. 이는 브라우저의 권한 정책에 의해 이후 요청이 차단될 수 있습니다.
🔧 서비스 워커 등록 후 토큰 요청 순서 보장 제안
- const requestFCMToken = useCallback(async (): Promise<string | null> => {
+ const requestFCMToken = useCallback(async (swRegistration?: ServiceWorkerRegistration): Promise<string | null> => {
try {
const app = getApps().length > 0 ? getApp() : initializeApp(firebaseConfig);
const messaging = getMessaging(app);
const permission = await Notification.requestPermission();
- registerServiceWorker();
if (permission !== 'granted') {
alert('알림 권한 허용이 필요합니다!');
return null;
}
const forFcmtoken = await getToken(messaging, {
vapidKey: import.meta.env.VITE_FIREBASE_VAPID_KEY,
+ serviceWorkerRegistration: swRegistration,
});useEffect 내에서 서비스 워커를 먼저 등록한 후 registration 객체를 requestFCMToken에 전달하는 방식을 권장합니다.
Also applies to: 66-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts` around lines
38 - 64, requestFCMToken currently calls registerServiceWorker() without
awaiting it causing a race with getToken; change requestFCMToken to await
service worker registration (or accept a ServiceWorkerRegistration parameter)
and pass that registration to getToken (use the messaging registration option)
so token retrieval happens after registration completes; also modify the
useEffect that triggers requestFCMToken on mount to avoid prompting for
notification permission immediately (invoke requestFCMToken only in response to
a user action or after explicit consent) and update any callers to provide the
registration if you switched to the parameter approach; refer to
requestFCMToken, registerServiceWorker, and the mounting useEffect when making
these changes.
Comment on lines
+90
to
+96
| if (isAlarmStep) { | ||
| if (alarmSelected === 1) setRemindTime('09:00'); | ||
| else if (alarmSelected === 2) setRemindTime('20:00'); | ||
| else { | ||
| const raw = AlarmsType[alarmSelected - 1].time; | ||
| setRemindTime(normalizeTime(raw)); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "alarms.ts" --type fRepository: Pinback-Team/pinback-client
Length of output: 106
🏁 Script executed:
fd "useOnboardingFunnel.ts" --type fRepository: Pinback-Team/pinback-client
Length of output: 132
🏁 Script executed:
rg "normalizeTime" --type ts --type tsx -A 10Repository: Pinback-Team/pinback-client
Length of output: 98
🏁 Script executed:
rg "normalizeTime" --type ts -A 10Repository: Pinback-Team/pinback-client
Length of output: 5227
🏁 Script executed:
cat apps/client/src/constants/alarms.tsRepository: Pinback-Team/pinback-client
Length of output: 545
🏁 Script executed:
cat -n apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts | sed -n '85,100p'Repository: Pinback-Team/pinback-client
Length of output: 682
🏁 Script executed:
cat apps/client/src/pages/onBoarding/utils/formatRemindTime.tsRepository: Pinback-Team/pinback-client
Length of output: 749
🏁 Script executed:
cat apps/client/src/constants/alarms.tsRepository: Pinback-Team/pinback-client
Length of output: 545
🏁 Script executed:
cat -n apps/client/src/pages/onBoarding/components/funnel/AlarmBox.tsx | grep -A 50 "select === 3"Repository: Pinback-Team/pinback-client
Length of output: 1465
🏁 Script executed:
cat -n apps/client/src/pages/onBoarding/components/funnel/AlarmBox.tsx | sed -n '1,50p'Repository: Pinback-Team/pinback-client
Length of output: 1866
🏁 Script executed:
rg "getTimePicker" apps/client/src/pages/onBoarding/components/funnel/AlarmBox.tsx -A 5 -B 5Repository: Pinback-Team/pinback-client
Length of output: 567
🏁 Script executed:
rg "AlarmStep" --type ts -A 30 | head -80Repository: Pinback-Team/pinback-client
Length of output: 7183
🏁 Script executed:
cat -n apps/client/src/pages/onBoarding/components/funnel/AlarmStep.tsxRepository: Pinback-Team/pinback-client
Length of output: 167
🏁 Script executed:
fd "AlarmStep" --type fRepository: Pinback-Team/pinback-client
Length of output: 140
🏁 Script executed:
rg "AlarmsType\[2\].time =" --type ts -B 5 -A 5Repository: Pinback-Team/pinback-client
Length of output: 1076
🏁 Script executed:
cat -n apps/client/src/pages/onBoarding/components/funnel/step/AlarmStep.tsxRepository: Pinback-Team/pinback-client
Length of output: 1556
🏁 Script executed:
rg "remindTime" apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts -B 2 -A 2Repository: Pinback-Team/pinback-client
Length of output: 598
alarmSelected === 3일 때 사용자 설정 시간을 선택하지 않은 경우 normalizeTime에 빈 문자열이 전달되어 'NaN:NaN'이 설정됩니다.
AlarmsType[2].time은 초기값이 ''(빈 문자열)입니다. 사용자가 선택지 3(사용자 설정)을 선택했지만 TimePicker를 통해 시간을 지정하지 않은 채로 다음 단계로 진행하면, normalizeTime('')이 호출됩니다. 이 함수는 빈 문자열을 분할하여 parseInt('', 10) → NaN을 반환하므로, remindTime이 'NaN:NaN'으로 설정되어 백엔드로 전송됩니다.
시간 선택 검증을 추가하거나, 빈 문자열 처리를 추가해야 합니다.
🐛 빈 문자열 가드 추가 제안
} else {
const raw = AlarmsType[alarmSelected - 1].time;
- setRemindTime(normalizeTime(raw));
+ if (raw) {
+ setRemindTime(normalizeTime(raw));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (isAlarmStep) { | |
| if (alarmSelected === 1) setRemindTime('09:00'); | |
| else if (alarmSelected === 2) setRemindTime('20:00'); | |
| else { | |
| const raw = AlarmsType[alarmSelected - 1].time; | |
| setRemindTime(normalizeTime(raw)); | |
| } | |
| if (isAlarmStep) { | |
| if (alarmSelected === 1) setRemindTime('09:00'); | |
| else if (alarmSelected === 2) setRemindTime('20:00'); | |
| else { | |
| const raw = AlarmsType[alarmSelected - 1].time; | |
| if (raw) { | |
| setRemindTime(normalizeTime(raw)); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts` around lines
90 - 96, When handling alarmSelected in useOnboardingFunnel, avoid calling
normalizeTime on an empty AlarmsType entry: check the computed raw =
AlarmsType[alarmSelected - 1].time for falsy/empty string before calling
normalizeTime; if raw is empty, do not call normalizeTime or set remindTime to a
safe default (e.g., '09:00') or trigger validation so you don't set remindTime
to 'NaN:NaN'; update the branch around isAlarmStep / alarmSelected to guard raw,
call normalizeTime(raw) only when raw is non-empty, and use setRemindTime with
the fallback or validation flow otherwise.
Comment on lines
+105
to
+117
| if (isFinalStep) { | ||
| postSignData( | ||
| { email: userEmail, remindDefault: remindTime, fcmToken }, | ||
| { | ||
| onSuccess: () => (window.location.href = '/'), | ||
| onError: () => { | ||
| const savedEmail = localStorage.getItem('email'); | ||
| if (savedEmail) window.location.href = '/'; | ||
| }, | ||
| } | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for postSignData API type definition and fcmToken usage
rg -n "postSignData|postSignUp" --type ts -C 3 | head -100Repository: Pinback-Team/pinback-client
Length of output: 3800
🏁 Script executed:
#!/bin/bash
# Find fcmToken initialization around Line 28
rg -n "fcmToken" apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts -C 5Repository: Pinback-Team/pinback-client
Length of output: 1214
🏁 Script executed:
#!/bin/bash
# Look for the API request type definition
fd -e ts -e tsx | xargs rg -l "fcmToken" | head -10Repository: Pinback-Team/pinback-client
Length of output: 202
🏁 Script executed:
#!/bin/bash
# Check how fcmToken is set - look for requestFcmToken or similar
rg -n "setFcmToken" apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts -C 10Repository: Pinback-Team/pinback-client
Length of output: 1481
🏁 Script executed:
#!/bin/bash
# Check if there are any FCM token request functions
rg -n "fcmToken|FCM|firebase" apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts -A 2Repository: Pinback-Team/pinback-client
Length of output: 1122
🏁 Script executed:
#!/bin/bash
# Check the complete onError handler and context
sed -n '105,120p' apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.tsRepository: Pinback-Team/pinback-client
Length of output: 480
🏁 Script executed:
#!/bin/bash
# Search for any error notifications or logging in the file
rg -n "toast|console|alert|error" apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.tsRepository: Pinback-Team/pinback-client
Length of output: 231
onError 핸들러에서 회원가입 실패를 무시하고 사용자에게 성공한 것처럼 보이게 합니다.
postSignData 실패 시 onError 핸들러(Line 110-113)가 savedEmail이 존재하면 에러를 무시하고 홈으로 리다이렉트합니다. 이는 회원가입이 실패했음에도 사용자에게 성공한 것처럼 보여 혼동을 야기할 수 있습니다. 에러 발생 시 사용자에게 알림을 표시하고 적절하게 처리해야 합니다.
참고: fcmToken이 null일 수 있는 것은 API 타입에서 의도적으로 허용(fcmToken: string | null)하고 있으므로 설계상 문제는 아니지만, FCM 요청 실패 시 사용자 경험을 개선할 필요가 있습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/client/src/pages/onBoarding/hooks/useOnboardingFunnel.ts` around lines
105 - 117, The onError block in the postSignData call inside useOnboardingFunnel
(the isFinalStep branch) incorrectly treats failures as success by redirecting
when a savedEmail exists; change onError to surface the failure instead of
silently redirecting: remove the conditional redirect, capture the error from
the postSignData onError callback and set a visible error state or call the
existing UI notification/toast utility (or expose setError via the hook) so the
UI can show an error message and let the user retry; keep the successful
onSuccess redirect intact and ensure fcmToken null handling remains unchanged.
Member
Author
There was a problem hiding this comment.
에러 fallback은 이후에 추가 예정
There was a problem hiding this comment.
@constantly-dev, 알겠습니다! 이후 PR에서 에러 fallback을 추가하실 계획이시군요.
이 작업을 추적하기 위해 GitHub 이슈를 생성해 드릴까요? 그러면 나중에 놓치지 않고 처리하실 수 있을 것 같습니다.
🧠 Learnings used
Learnt from: constantly-dev
Repo: Pinback-Team/pinback-client PR: 2
File: pnpm-workspace.yaml:3-3
Timestamp: 2025-08-18T13:48:59.065Z
Learning: constantly-dev는 docs 디렉터리를 컨벤션 문서 추가용으로 사용할 예정이라고 명시했습니다.
Learnt from: constantly-dev
Repo: Pinback-Team/pinback-client PR: 5
File: apps/extension/src/index.css:1-1
Timestamp: 2025-08-19T17:18:57.678Z
Learning: constantly-dev는 디자인 시스템 설정 PR 머지 후 `import 'pinback/tailwind-config/shared-styles.css';`를 `app.css`나 `index.css`에서 사용하여 공유 스타일을 관리할 계획입니다.
This was referenced Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Related Issues
📄 Tasks
⭐ PR Point (To Reviewer)
1. 목적
기존 mainCard와 이번에 직무 부분에 추가되는 구조도 funnel 구조라 공통으로 사용가능한 useFunnel 훅을 추가 및 분리했어요.
그리고 기존과 동일하게
searchParams를 통해 step을 관리하도록 하였어요. 이때 따로 step 상태를 추가하지 않고, 오직searchParams에서만 관리하도록 해서 단일 진실 공급원 원칙을 지키도록 설계했어요. 이에 따라 더 상태 추적이 쉽도록 했어요.2. useFunnel interface
해당 훅의 interface는 아래와 같아요.
step들을 배열로 받는 steps, 초기 step을 string으로 받고 대부분의 경우 step으로 쿼리 파라미터 key를
?step=JOB과 같이 step으로 두긴하겠지만 특정 도메인에 종속되지 않도록querykey지정도 할 수 있도록 인터페이스를 열어뒀어요. (defaultValue = 'step')추가로 step을 이동하는
setStep,goNext,goPrev에는replaceoption을 선택에 따라 사용할 수 있어요. 필요 시에isFirstStep,isLastStep도 사용할 수 있도록 return에 추가해뒀어요.3. useOnboardingFunnel hook 2차 추상화
useFunnel공통 훅을 구현하고 분리하긴 했지만 여전히mainCard(onboarding)에는 기기에 따른 mac step 분기처리, final step 분기 등 온보딩 도메인에 종속된 funnel 로직이 남아있었어요.따라서 이를
useFunnel을 활용한 2차 추상화를 통해useOnboardingFunnel를 만들었어요. 단순한 물리적인 코드 위치 변경을 위한 분리였다면 추상화에 대한 의미가 퇴색될 수 있다고 생각하지만, 적절하게 책임을 분리하고 온보딩 funnel 로직만을 따로 분리해서 최대한 유의미한 접근이 되도록 해봤어요. (피드백 너무 환영입니다!!)📷 Screenshot
Summary by CodeRabbit
릴리스 노트