[feat] 알림 페이지 구현#41
Open
limtjdghks wants to merge 16 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthrough알림 타입·API, React Query 훅, 뷰모델, 알림 목록/설정 페이지, Toggle·NotificationItem UI, 라우팅 통합과 Storybook 설정이 추가되었습니다. Changes알림 기능 추가
🎯 Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 24.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목이 알림 페이지 구현이라는 주요 변경 내용을 정확히 반영하고 있으며, 간결하고 명확합니다. |
| Description check | ✅ Passed | PR 설명이 템플릿 모든 필수 섹션(ID, 변경 내용, 구현 사항)을 포함하고 있으며, 상세한 구현 내용과 데모 영상, 알려진 미완료 사항까지 명시하고 있습니다. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
feat/ALT-232
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/app/App.tsx (1)
29-30: ⚡ Quick win신규 알림 페이지 라우트도 lazy/Suspense로 분리해주세요.
NotificationPage,NotificationSettingsPage가 동기 import라 초기 번들에 포함됩니다. 기존SignupPage처럼 lazy route로 맞추는 게 일관성과 초기 로딩에 유리합니다.As per coding guidelines "
src/app/**: 라우팅 구조가 lazy loading을 활용하는지 확인" 및 "src/pages/**: React.lazy / Suspense를 통한 코드 스플리팅 적용 여부".Also applies to: 103-107
🤖 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/app/App.tsx` around lines 29 - 30, NotificationPage and NotificationSettingsPage are currently imported synchronously and should be converted to lazy-loaded routes to avoid including them in the initial bundle; replace their direct imports with React.lazy wrappers (e.g., const NotificationPage = React.lazy(() => import('...NotificationPage')) and const NotificationSettingsPage = React.lazy(() => import('...NotificationSettingsPage'))), then ensure the routes that render NotificationPage and NotificationSettingsPage are wrapped with a Suspense fallback (same pattern used for SignupPage) so code-splitting is applied consistently.src/pages/notification/index.tsx (1)
60-75: 🏗️ Heavy lift페이지에서 무한스크롤 관찰 로직을 분리해주세요.
IntersectionObserver생성/해제 로직이 페이지에 들어와 있어 페이지 조합 책임을 넘습니다.useNotificationViewModel또는 별도 hook으로 옮겨 페이지는 렌더링 조합만 담당하게 유지하는 편이 좋습니다.As per coding guidelines "
src/pages/**: 페이지 컴포넌트가 비즈니스 로직 없이 조합(Composition)만 하는지".🤖 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/notification/index.tsx` around lines 60 - 75, Move the IntersectionObserver creation/teardown out of the page component and into the notification view model or a new custom hook (e.g. useNotificationViewModel or useInfiniteScrollObserver) so the page only composes UI; specifically, take the useEffect block that references sentinelRef, creates new IntersectionObserver, observes el, and disconnects on cleanup, and implement it inside the view model/hook so it accepts sentinelRef (or a ref setter) and the control flags hasNextPage, isFetchingNextPage and action fetchNextPage; ensure the observer callback uses those flags and that the hook returns any necessary wiring for the page (e.g. sentinelRef or ref callback) and that cleanup is performed on unmount.
🤖 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/features/notification/hooks/useNotifications.ts`:
- Line 19: NotificationPage's cursor is currently typed as string but must allow
null per API behavior; open the NotificationPage type in the notification types
(NotificationPage) and change the cursor property to string | null (matching
other page DTOs like ChatRoomPageDto/ApplicationPageDto/PostingPageDto), then
run a quick typecheck and adjust any callsites that assume a non-null cursor
(e.g., pagination helpers such as getNextPageParam) to handle null/undefined
accordingly.
In `@src/features/notification/hooks/useUpdateNotificationConsent.ts`:
- Around line 8-14: useUpdateNotificationConsent currently chooses updater based
on scope but lets scope=null implicitly fall back to
updateUserNotificationConsent; change the mutation to explicitly handle null by
preventing execution when scope is null: inside useUpdateNotificationConsent set
the useMutation's mutationFn to undefined (or an early-returning function) when
scope === null, or add a guard that throws/returns early so callers cannot call
mutate with scope null; update references to updater, useMutation,
updateManagerNotificationConsent and updateUserNotificationConsent accordingly
and ensure callers either never call mutate when scope is null or check
mutation.isIdle before invoking.
In `@src/features/notification/types/consent.ts`:
- Around line 18-21: Update the UpdateNotificationConsentRequest interface so
the type field is constrained to the CONSENT_TYPE union instead of plain string:
replace type: string with type: CONSENT_TYPE (or the appropriate union/enum
exported as CONSENT_TYPE) and ensure you import or reference CONSENT_TYPE in
this module; update any usages that construct UpdateNotificationConsentRequest
to use a valid CONSENT_TYPE value.
In `@src/features/notification/useNotificationSettingsViewModel.ts`:
- Around line 23-24: The substituteEnabled and reputationEnabled toggles in
useNotificationSettingsViewModel are only local state and reset to true on
refresh; persist them to avoid user confusion by reading initial values from
localStorage and writing updates back whenever setSubstituteEnabled or
setReputationEnabled change (use useEffect to sync), using distinct localStorage
keys (e.g., "notification.substituteEnabled" and
"notification.reputationEnabled") and fall back to true if absent; if you prefer
not to persist because the API is missing, update the UI behavior in the same
hook to expose a "coming soon"/"not saved" flag instead of implying the toggles
are saved.
In `@src/features/notification/useNotificationViewModel.ts`:
- Around line 56-63: The currentItems value is not being filtered by activeTab
so tabs don't change displayed items; update the logic that computes
currentItems (and the derived hasUnreadSubstitute and hasUnreadReputation) to
filter the full notifications list by activeTab (use the item's title or
category to distinguish '대타' vs '평판'), return only the items matching the
activeTab for currentItems, and compute hasUnreadSubstitute/hasUnreadReputation
from the filtered lists (keep using setActiveTab as-is). Ensure you reference
and update the existing symbols currentItems, activeTab, hasUnreadSubstitute,
hasUnreadReputation when adding the filter logic.
In `@src/pages/notification/index.tsx`:
- Around line 130-131: 현재 currentItems.map(...)에서 <li key={item.id ?? idx}>처럼
index를 fallback으로 사용하고 있어 삭제/페이지네이션 시 DOM 재사용 문제가 발생할 수 있습니다; 이 문제를 해결하려면 렌더링할 때
반드시 안정적인 고유 key만 사용하도록 item.id를 필수화하거나 렌더 전에 id가 없는 항목을 필터링하세요 (예: 처리 로직에서
currentItems를 생성하는 곳 또는 컴포넌트 내부에서 map 호출 전에 id가 없는 항목을 제거하거나 데이터 유효성 검사 추가).
currentItems, item.id, 그리고 해당 map 渲染 블록을 찾아 수정하세요.
In `@src/shared/ui/notification/NotificationItem.tsx`:
- Around line 84-133: The onClick can fire after a swipe because touchend is
followed by a click; track a didDrag flag and ignore clicks when a drag
happened. Modify startDrag/moveDrag/endDrag (and their callers
handleTouchStart/handleTouchMove/handleTouchEnd and handleMouseDown's
onMove/onUp) to set didDrag = false at start, set didDrag = true when moveDrag
detects sufficient movement, and reset didDrag to false after endDrag completes;
then in the button onClick handler check didDrag and return early if true
(instead of only checking offset). Use the existing functions startDrag,
moveDrag, endDrag and state setters like setOffset to locate where to add and
clear the didDrag flag.
- Around line 90-102: handleMouseDown currently attaches document 'mousemove'
and 'mouseup' handlers that are only removed in the 'mouseup' path, which leaks
listeners if the component unmounts mid-drag; fix by storing the onMove and onUp
listener references in refs (e.g. moveListenerRef, upListenerRef) when you
create them in handleMouseDown, and add a useEffect cleanup that checks those
refs and calls document.removeEventListener('mousemove',
moveListenerRef.current) and document.removeEventListener('mouseup',
upListenerRef.current) and clears the refs; ensure you still call endDrag() in
cleanup and keep using startDrag, moveDrag and endDrag as before so the drag
state is correctly finalized.
---
Nitpick comments:
In `@src/app/App.tsx`:
- Around line 29-30: NotificationPage and NotificationSettingsPage are currently
imported synchronously and should be converted to lazy-loaded routes to avoid
including them in the initial bundle; replace their direct imports with
React.lazy wrappers (e.g., const NotificationPage = React.lazy(() =>
import('...NotificationPage')) and const NotificationSettingsPage =
React.lazy(() => import('...NotificationSettingsPage'))), then ensure the routes
that render NotificationPage and NotificationSettingsPage are wrapped with a
Suspense fallback (same pattern used for SignupPage) so code-splitting is
applied consistently.
In `@src/pages/notification/index.tsx`:
- Around line 60-75: Move the IntersectionObserver creation/teardown out of the
page component and into the notification view model or a new custom hook (e.g.
useNotificationViewModel or useInfiniteScrollObserver) so the page only composes
UI; specifically, take the useEffect block that references sentinelRef, creates
new IntersectionObserver, observes el, and disconnects on cleanup, and implement
it inside the view model/hook so it accepts sentinelRef (or a ref setter) and
the control flags hasNextPage, isFetchingNextPage and action fetchNextPage;
ensure the observer callback uses those flags and that the hook returns any
necessary wiring for the page (e.g. sentinelRef or ref callback) and that
cleanup is performed on unmount.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 52985ef4-a7d0-44e7-ae4c-1ede75e2abf3
⛔ Files ignored due to path filters (2)
src/assets/alter-logo-vector.svgis excluded by!**/*.svgsrc/assets/icons/settings.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
src/app/App.tsxsrc/features/notification/api/notificationConsent.tssrc/features/notification/api/notifications.tssrc/features/notification/hooks/useNotificationConsent.tssrc/features/notification/hooks/useNotifications.tssrc/features/notification/hooks/useUpdateNotificationConsent.tssrc/features/notification/types/consent.tssrc/features/notification/types/index.tssrc/features/notification/useNotificationSettingsViewModel.tssrc/features/notification/useNotificationViewModel.tssrc/pages/notification/index.tsxsrc/pages/notification/settings/index.tsxsrc/shared/constants/routes.tssrc/shared/lib/queryKeys.tssrc/shared/ui/common/Navbar.tsxsrc/shared/ui/common/Toggle.tsxsrc/shared/ui/notification/NotificationItem.tsxstorybook/stories/NotificationItem.stories.tsxtailwind.config.js
dohy-eon
reviewed
May 20, 2026
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/features/notification/useNotificationSettingsViewModel.ts (1)
24-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick win대타/평판 토글 상태가 새로고침 시 유실됩니다.
substituteEnabled,reputationEnabled가useState(true)로 고정 초기화되어 사용자 변경값이 유지되지 않습니다. API 미지원 상태라면localStorage동기화 또는 “저장되지 않음/준비 중” 상태를 명시해 오해를 막아주세요.수정 예시
- const [substituteEnabled, setSubstituteEnabled] = useState(true) - const [reputationEnabled, setReputationEnabled] = useState(true) + const [substituteEnabled, setSubstituteEnabled] = useState( + () => localStorage.getItem('notification.substituteEnabled') !== 'false' + ) + const [reputationEnabled, setReputationEnabled] = useState( + () => localStorage.getItem('notification.reputationEnabled') !== 'false' + ) + + const handleSubstituteEnabledChange = (checked: boolean) => { + setSubstituteEnabled(checked) + localStorage.setItem('notification.substituteEnabled', String(checked)) + } + + const handleReputationEnabledChange = (checked: boolean) => { + setReputationEnabled(checked) + localStorage.setItem('notification.reputationEnabled', String(checked)) + }🤖 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/features/notification/useNotificationSettingsViewModel.ts` around lines 24 - 25, substituteEnabled and reputationEnabled are hard-coded to true and lose user changes on refresh; initialize them from localStorage (e.g. keys like "notification.substituteEnabled" and "notification.reputationEnabled") instead of useState(true), and add an effect to persist updates via setSubstituteEnabled/setReputationEnabled to localStorage whenever they change; if the API is not available, surface an explicit "unsaved/preview" state (e.g. a boolean flag) so users know changes are local-only instead of silently reverting.
🤖 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.
Duplicate comments:
In `@src/features/notification/useNotificationSettingsViewModel.ts`:
- Around line 24-25: substituteEnabled and reputationEnabled are hard-coded to
true and lose user changes on refresh; initialize them from localStorage (e.g.
keys like "notification.substituteEnabled" and "notification.reputationEnabled")
instead of useState(true), and add an effect to persist updates via
setSubstituteEnabled/setReputationEnabled to localStorage whenever they change;
if the API is not available, surface an explicit "unsaved/preview" state (e.g. a
boolean flag) so users know changes are local-only instead of silently
reverting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 22e59437-784c-435f-b24c-6eb2f1d9fba1
📒 Files selected for processing (6)
src/features/notification/hooks/useUpdateNotificationConsent.tssrc/features/notification/types/consent.tssrc/features/notification/types/index.tssrc/features/notification/useNotificationSettingsViewModel.tssrc/pages/notification/settings/index.tsxsrc/shared/ui/notification/NotificationItem.tsx
dohy-eon
requested changes
May 21, 2026
Member
dohy-eon
left a comment
dohy-eon
left a comment
There was a problem hiding this comment.
탭이랑 필터링 로직 추가 + 코멘트 읽어보시구 처리해주심 감사드리겠습니다.
| function mapDto(dto: NotificationDto): Omit<NotificationItemProps, 'onDelete'> { | ||
| return { | ||
| id: dto.id, | ||
| isRead: false, |
Member
There was a problem hiding this comment.
해당부분 읽음처리가 false 고정인데 백엔드 dto에 혹시 isRead나 readAt dto가 없는걸까요?
Member
There was a problem hiding this comment.
알림에도 미읽음 표시(mainColor or error색상)가 들어가야 할 것 같은데.. 처리 가능하실까요 ?
Member
Author
There was a problem hiding this comment.
동그랗게 뱃지 추가해달라는 말씀이시죠? 근데 그럼 알림 조회 로직을 어떤식으로 구성해야 할까요..
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.
ID
변경 내용
구현 사항
알림 아이템 컴포넌트 (NotificationItem)
알림 페이지 (/notifications)
알림 설정 페이지 (/notifications/settings)
API 연동
구현 시연 (필요 시)
2026-05-20.10.08.43.mov
참고 사항 (필요 시)
필요한 API
디자인 수정되어야하는거
Summary by CodeRabbit