Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthrough이 PR은 알림 관련 분석 추적을 추가하고 알림 데이터/컴포넌트 인터페이스를 확장합니다. 홈 페이지의 알람 아이콘 클릭에서 ALARM_ICON_CLICK 이벤트를 전송하도록 했고, 알림 페이지에 PAGE_VIEW, NOTIFICATION_CLICK, NOTIFICATION_READ 이벤트를 추가했습니다. 또한 notification DTO 매퍼에 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/web/src/app-pages/notification/ui/NotificationPage.tsx`:
- Around line 87-88: The tracking currently sends notification_id as
`noti_${id}` in the trackNotificationEvent call
(NOTIFICATION_EVENTS.NOTIFICATION_READ), which diverges from the server's
numeric id; change the tracked payload to send the numeric id (notification_id:
id) to match API/server logs, and update any other occurrences (e.g., other
events, admin app, shared packages) to the same numeric-id convention; if you
opt for a different canonical format instead, document and apply that format
consistently across trackNotificationEvent usage and related analytics helpers.
- Around line 85-89: The current code calls readNotification(id) then
immediately calls
trackNotificationEvent(NOTIFICATION_EVENTS.NOTIFICATION_READ...), but
readNotification is an async React Query mutation; change to call
readNotification via its mutate (or mutateAsync) and move the
trackNotificationEvent into the mutation's success callback so the
NOTIFICATION_READ event is only logged after the mutation succeeds; locate the
readNotification usage in NotificationPage.tsx and use
readNotification.mutate(id, { onSuccess: () => { trackNotificationEvent(...) }
}) or await readNotification.mutateAsync(id) and then call
trackNotificationEvent.
🧹 Nitpick comments (3)
apps/web/src/entities/notification/ui/NotificationItem.tsx (1)
8-18: notificationType 타입을 도메인 타입으로 고정 권장.
- 문제:
string이라 유효하지 않은 값이 들어와도 컴파일 단계에서 잡히지 않습니다.- 원인:
notificationType가 느슨한 타입으로 정의됨.- 대안: 도메인 타입(예:
Notification['type']또는 별도NotificationType유니온)을 공유해 타입 안정성을 확보.- 장점: 추적 이벤트/렌더링에서 잘못된 타입 유입 방지.
- 단점: 타입 정의(또는 import) 추가가 필요.
♻️ 예시 변경안
+import type { Notification } from '../api/types'; ... export interface NotificationItemProps { ... - notificationType: string; + notificationType: Notification['type']; }apps/web/src/app-pages/notification/ui/NotificationPage.tsx (2)
55-59: PAGE_VIEW 트래킹 로직의 의존성 배열 점검 필요현재
useEffect의 의존성 배열에pageName이 포함되어 있지만,trackRef가드로 인해 최초 1회만 실행됩니다. 이로 인해 두 가지 잠재적 문제가 있습니다:
- 초기값 문제:
usePageName()이 초기에 빈 값이나 다른 값을 반환하고 이후 변경되는 경우, 첫 번째 값으로만 이벤트가 발생합니다.- 린트 경고 가능성:
trackRef를 의존성에서 사용하지만, 실제로는 deps 변경과 무관하게 동작하여 ESLint exhaustive-deps 규칙과 충돌할 수 있습니다.의도가 "마운트 시 1회 트래킹"이라면, 의존성 배열을 빈 배열로 변경하고
pageName이 안정적인 값인지 확인하거나,pageName이 유효할 때만 트래킹하도록 조건을 추가하는 것이 더 명확합니다.♻️ 개선 제안: 명시적인 마운트 트래킹
useEffect(() => { if (trackRef.current) return; + if (!pageName) return; // pageName이 유효할 때만 트래킹 trackRef.current = true; trackNotificationEvent(NOTIFICATION_EVENTS.PAGE_VIEW, { page_name: pageName }); - }, [pageName]); + }, [pageName]); // pageName 변경 시 재실행 가능하도록 유지하거나, [] 로 변경또는 마운트 1회만 필요하다면:
useEffect(() => { - if (trackRef.current) return; - trackRef.current = true; trackNotificationEvent(NOTIFICATION_EVENTS.PAGE_VIEW, { page_name: pageName }); + // eslint-disable-next-line react-hooks/exhaustive-deps - }, [pageName]); + }, []); // 마운트 시 1회만 실행
74-97: handleNotificationClick 함수 구조 개선 제안현재 함수가 여러 책임(트래킹, 읽음 처리, 네비게이션)을 가지고 있어 점점 복잡해지고 있습니다. 가독성과 테스트 용이성을 위해 각 동작을 분리하는 것을 고려해 보세요.
현재 구조가 동작에는 문제없지만, 향후 추가 로직이 생길 경우 유지보수가 어려워질 수 있습니다.
♻️ 선택적 개선: 책임 분리
// 트래킹 로직 분리 (features 레이어로 이동 가능) const trackNotificationInteraction = ( id: number, notificationType: string, isRead: boolean, ) => { trackNotificationEvent(NOTIFICATION_EVENTS.NOTIFICATION_CLICK, { notification_type: notificationType, }); if (!isRead) { trackNotificationEvent(NOTIFICATION_EVENTS.NOTIFICATION_READ, { notification_id: `noti_${id}`, }); } }; const handleNotificationClick = ( id: number, deepLink: string, isRead: boolean, notificationType: string, ) => { trackNotificationInteraction(id, notificationType, isRead); if (!isRead) { readNotification(id); } if (deepLink) { const separator = deepLink.includes('?') ? '&' : '?'; router.push(`${deepLink}${separator}from=notification`); } };
| trackNotificationEvent(NOTIFICATION_EVENTS.NOTIFICATION_READ, { | ||
| notification_id: `noti_${id}`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "notification_id" apps/web/src/Repository: Tave-Makers/SURF-FE
Length of output: 302
🏁 Script executed:
rg -n "trackNotificationEvent|NOTIFICATION_EVENTS" apps/web/src/ -A 3Repository: Tave-Makers/SURF-FE
Length of output: 4105
🏁 Script executed:
fd -e ts -e tsx . packages/ apps/web/src | xargs rg -l "notification.*id|notif.*id" -i | head -20Repository: Tave-Makers/SURF-FE
Length of output: 367
🏁 Script executed:
cat -n apps/web/src/entities/notification/api/readNotification.tsRepository: Tave-Makers/SURF-FE
Length of output: 303
🏁 Script executed:
cat -n apps/web/src/entities/notification/ui/NotificationList.tsx | head -100Repository: Tave-Makers/SURF-FE
Length of output: 817
🏁 Script executed:
# 다른 이벤트 추적에서 ID 포맷 패턴 확인
rg -n "_\$\{|prefix|format" apps/web/src/features/notification-tracking/ apps/web/src/app-pages/Repository: Tave-Makers/SURF-FE
Length of output: 45
🏁 Script executed:
# notification 엔터티의 타입 정의 확인
fd -e ts -e tsx . apps/web/src/entities/notification | xargs catRepository: Tave-Makers/SURF-FE
Length of output: 11258
추적 이벤트의 ID 형식을 서버/분석 시스템과 일치시키세요
서버는 id: number 형식으로 반환하지만, 추적 시에만 noti_${id} 접두사를 붙여 전송하고 있습니다. API 호출 시에는 숫자 ID를 그대로 사용하므로, 이 포맷 차이로 인해 추후 분석 데이터와 서버 로그의 조인이 어려워질 수 있습니다.
분석팀과 협의하여:
- 접두사를 제거하고 숫자만 전송할지
- 아니면 모든 이벤트에 일관된 ID 포맷 규칙을 문서화할지
결정하고 적용하세요. 특히 다른 앱(예: admin)이나 패키지에서도 같은 규칙을 따르도록 표준화가 필요합니다.
🤖 Prompt for AI Agents
In `@apps/web/src/app-pages/notification/ui/NotificationPage.tsx` around lines 87
- 88, The tracking currently sends notification_id as `noti_${id}` in the
trackNotificationEvent call (NOTIFICATION_EVENTS.NOTIFICATION_READ), which
diverges from the server's numeric id; change the tracked payload to send the
numeric id (notification_id: id) to match API/server logs, and update any other
occurrences (e.g., other events, admin app, shared packages) to the same
numeric-id convention; if you opt for a different canonical format instead,
document and apply that format consistently across trackNotificationEvent usage
and related analytics helpers.
| trackNotificationEvent(NOTIFICATION_EVENTS.NOTIFICATION_READ, { | ||
| notification_id: `noti_${id}`, | ||
| }); |
There was a problem hiding this comment.
id에서 noti를 빼고 보내도 될 것 같은데 어떨까요?
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/web/src/app-pages/notification/ui/NotificationPage.tsx`:
- Around line 86-92: The onSuccess callback for readNotification now correctly
defers tracking until the mutation succeeds and the event payload uses the
server ID format; ensure the call to readNotification includes onSuccess that
calls trackNotificationEvent(NOTIFICATION_EVENTS.NOTIFICATION_READ, {
notification_id: `${id}` }) (no "noti_" prefix) so events fire only on
successful mutation—verify the readNotification invocation and the referenced
symbols (readNotification, onSuccess, trackNotificationEvent,
NOTIFICATION_EVENTS.NOTIFICATION_READ, id) match this behavior across
NotificationPage.tsx.
2 participants
✅ 체크리스트
🔗 관련 이슈
📌 작업 목적
🔨 주요 작업 내용
🧪 테스트 방법
✔️ 설치 라이브러리
📷 실행 영상 또는 스크린샷
💬 논의할 점
🙋🏻 참고 자료
Summary by CodeRabbit
릴리스 노트
New Features
Chores