refactor: 알림 관련 파일 리팩토링 - #42
Merged
Merged
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough알림 패널을 Changes알림 기능
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Header
participant NotificationTrigger
participant NotificationPanel
participant NotificationsAPI
Header->>NotificationTrigger: 알림 버튼 표시
NotificationTrigger->>NotificationPanel: 패널 열기
NotificationPanel->>NotificationsAPI: 페이지별 알림 조회
NotificationsAPI-->>NotificationPanel: 알림 목록과 페이지 정보 반환
NotificationPanel->>NotificationsAPI: 개별 알림 읽음 처리
NotificationsAPI-->>NotificationPanel: 갱신된 알림 반환
NotificationPanel-->>NotificationTrigger: 닫기 처리
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (2)
src/components/common/Header/notification/NotificationPagination.tsx (1)
28-67: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win페이지 수가 많아지면 페이지네이션이 넘칠 수 있습니다.
Array.from({ length: pageCount }, ...)는 페이지 번호 버튼을 모두 렌더링합니다.<ul>에는flex-wrap이나overflow-x-auto가 없습니다.NotificationPanel의 폭은w-[359px]로 고정되어 있습니다. 페이지 수가 늘어나면 버튼이 패널 폭을 초과해 잘리거나 넘칠 수 있습니다.가로 스크롤 처리나 페이지 윈도잉(예: 현재 페이지 주변만 표시하고 생략 부호 사용)을 추가하는 것을 권장합니다.
♻️ 최소 수정 예시: 가로 스크롤 허용
- <ul className="flex items-center gap-4"> + <ul className="flex items-center gap-4 overflow-x-auto">🤖 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/common/Header/notification/NotificationPagination.tsx` around lines 28 - 67, Update the pagination list in NotificationPagination to prevent page buttons from exceeding the fixed panel width: add horizontal overflow handling to the <ul> or replace the full pageCount rendering with a windowed page list that includes ellipses. Preserve current-page selection, navigation callbacks, and disabled states.src/components/common/Header/notification/NotificationPanel.tsx (1)
65-78: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
aria-modal="true"인데 배경 스크롤 잠금이 없습니다.경로 지침은 모달에 대해
role="dialog",aria-modal, 포커스 트랩, ESC 닫기, 배경 스크롤 잠금을 요구하며Modal.tsx를 참고 사례로 지정합니다. 이 패널은 포커스 트랩과 ESC 닫기는 구현했지만 배경 스크롤 잠금은 없습니다.aria-modal="true"로 선언하면 스크린 리더 사용자는 배경이 비활성화된 것으로 인식하지만, 실제로는 배경 스크롤이 가능해 선언된 상태와 동작이 불일치합니다.
Modal.tsx와 동일한 스크롤 잠금을 추가하거나, 드롭다운형 패널로 의도했다면aria-modal값을 재검토하십시오.As per path instructions, "모달은
role="dialog",aria-modal, 포커스 트랩, ESC 닫기, 배경 스크롤 잠금이 필요합니다.Modal.tsx가 참고 사례입니다."🤖 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/common/Header/notification/NotificationPanel.tsx` around lines 65 - 78, Update the notification panel component around its dialog state and the existing panelRef handling to add background scroll locking while the panel is open, matching the established behavior in Modal.tsx. Ensure the lock is applied on open and reliably removed on close or unmount, while preserving the existing focus-trap and ESC-dismiss behavior.Source: Path instructions
🤖 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/components/common/Header/notification/NotificationPanel.tsx`:
- Around line 48-63: Update handleNotificationActivate to catch and absorb
errors from await markAsRead(notification.id), while retaining the existing
finally cleanup of pendingReadIds. Do not rethrow the mutation failure, since
the existing onError handler already restores the cache.
- Around line 24-46: Update the NotificationPanel pagination flow around
currentPage, pageCount, and safePage so that when the fetched pagination
metadata shows currentPage exceeds pageCount, the current page state is
automatically reset to pageCount. Ensure the correction occurs after pageCount
is derived and avoids unnecessary state updates, while preserving the existing
bounded goToPage behavior.
In `@src/hooks/useFocusTrap.ts`:
- Around line 22-76: Update the focus-trapping effect in useFocusTrap to capture
document.activeElement before moving focus into the container, then restore that
element during cleanup when it is still connected to the document. Preserve the
existing keydown handling and focus behavior, including the container fallback
when no focusable elements exist.
In `@src/lib/utils/focusable.ts`:
- Around line 1-11: Update getFocusableElements to return only elements eligible
for Tab traversal by requiring element.tabIndex >= 0, which excludes
tabindex="-1" buttons and links. Also exclude descendants of disabled fieldsets
using element.matches(":disabled"), while preserving the existing hidden,
aria-hidden, and disabled-element filtering.
---
Nitpick comments:
In `@src/components/common/Header/notification/NotificationPagination.tsx`:
- Around line 28-67: Update the pagination list in NotificationPagination to
prevent page buttons from exceeding the fixed panel width: add horizontal
overflow handling to the <ul> or replace the full pageCount rendering with a
windowed page list that includes ellipses. Preserve current-page selection,
navigation callbacks, and disabled states.
In `@src/components/common/Header/notification/NotificationPanel.tsx`:
- Around line 65-78: Update the notification panel component around its dialog
state and the existing panelRef handling to add background scroll locking while
the panel is open, matching the established behavior in Modal.tsx. Ensure the
lock is applied on open and reliably removed on close or unmount, while
preserving the existing focus-trap and ESC-dismiss behavior.
🪄 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 Plus
Run ID: 6ee7265b-e638-4e40-afad-66aa0a7358e3
📒 Files selected for processing (18)
src/components/common/Header/Header.tsxsrc/components/common/Header/NotificationPanel.tsxsrc/components/common/Header/notification/NotificationItem.tsxsrc/components/common/Header/notification/NotificationPagination.tsxsrc/components/common/Header/notification/NotificationPanel.tsxsrc/components/common/Header/notification/NotificationTrigger.tsxsrc/components/common/Header/notification/index.tssrc/components/common/Header/notification/notificationMessages.tssrc/components/common/Modal.tsxsrc/hooks/notifications/useNotifications.tssrc/hooks/notifications/useReadNotification.tssrc/hooks/notifications/useUnreadNotificationCount.tssrc/hooks/useFocusTrap.tssrc/lib/api/notifications.tssrc/lib/constants/apiRoutes.tssrc/lib/mocks/notifications.mock.tssrc/lib/utils/focusable.tssrc/types/notification.ts
💤 Files with no reviewable changes (5)
- src/types/notification.ts
- src/lib/constants/apiRoutes.ts
- src/lib/api/notifications.ts
- src/components/common/Header/NotificationPanel.tsx
- src/lib/mocks/notifications.mock.ts
juengseulki
reviewed
Jul 31, 2026
juengseulki
left a comment
juengseulki
left a comment
Contributor
There was a problem hiding this comment.
📋 PR 리뷰
👍 좋았던 점
- Notification 관련 컴포넌트와 훅을
notification모듈로 정리해 역할이 훨씬 명확해졌습니다. NotificationItem,NotificationPagination으로 UI를 분리하면서NotificationPanel이 조합 역할만 담당하도록 개선했습니다.- 알림 조회, 읽음 처리, 미읽음 개수 훅을
hooks/notifications로 모아 관련 로직을 한 곳에서 관리할 수 있도록 정리했습니다. useFocusTrap과getFocusableElements를 공통화해 Modal과 NotificationPanel이 동일한 포커스 정책을 사용할 수 있도록 개선한 점이 좋았습니다.- 사용하지 않는 API, 타입, mock 데이터를 함께 제거해 죽은 코드까지 정리한 점도 좋았습니다.
접근성 측면에서 Escape 종료, 포커스 복귀, 페이지 상태 안내 등을 함께 반영해 사용자 경험을 개선했습니다.
🔍 확인 및 제안
useFocusTrap을 공통 훅으로 분리한 만큼 이후 Drawer, Dropdown 등 포커스 트랩이 필요한 컴포넌트에서도 동일한 훅을 사용할 계획인지 궁금합니다.NotificationPanel의 역할이 렌더링 조합에 집중되면서 구조가 훨씬 명확해졌습니다.Modal도 동일한getFocusableElements유틸을 사용하도록 변경되어 포커스 정책을 한 곳에서 관리할 수 있게 된 점이 좋았습니다.
전체적으로 기능 변경보다는 Notification 모듈의 구조 개선과 접근성 공통화에 초점을 맞춘 리팩토링이었습니다. 역할 분리가 잘 이루어졌고, 불필요한 코드까지 함께 정리되어 유지보수성이 좋아진 PR이라고 생각합니다. 수고하셨습니다! 😊
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
|
리팩토링하느라 고생 많으셨습니다! |
This was referenced Aug 4, 2026
Merged
6 tasks
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
3 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.
📋 작업 내용
알림 작업까지 완료하고 전체 검토하여 추가 리팩토링이 필요한 부분 진행
🔥 변경 사항
Header.tsx: 알림 트리거 import 경로를Header/notification으로 변경NotificationPanel.tsx(삭제→notification/로 재생성): 목록·페이지네이션을 분리하고 포커스 트랩을 맡도록 옮김NotificationTrigger.tsx→notification/NotificationTrigger.tsx: 열림/닫힘·포커스 복귀만 담당하도록 옮기고 Escape는 Panel로 넘김notificationMessages.ts→notification/notificationMessages.ts: 알림 문구 템플릿을 notification 폴더로 이동NotificationItem.tsx(신규): 알림 한 줄 UI와 메시지/a11y 처리를 분리NotificationPagination.tsx(신규): 알림 패널 페이지네이션 UI를 분리notification/index.ts(신규): notification 모듈 barrel export를 추가useNotifications.ts→hooks/notifications/: 목록 조회 훅을 notifications 폴더로 이동useReadNotification.ts→hooks/notifications/: 읽음 mutation 훅을 notifications 폴더로 이동useUnreadNotificationCount.ts→hooks/notifications/: 미읽음 수 훅을 notifications 폴더로 이동useFocusTrap.ts(신규): 포커스 이동·Tab 트랩·Escape 공통 훅을 추가focusable.ts(신규): 포커스 가능 요소 조회 유틸을 추가Modal.tsx: 로컬 포커스 헬퍼 대신 공통getFocusableElements를 쓰도록 변경notifications.ts(api): 미사용readAllNotifications를 제거apiRoutes.ts: 미사용READ_ALL경로를 제거notification.ts(types): 미사용ReadAllNotificationsResponse타입을 제거notifications.mock.ts(삭제): 더 이상 쓰이지 않는 mock 데이터를 삭제✅ 체크리스트
📷 스크린샷 (선택)
🔗 관련 이슈
Closes #
💬 To Reviewer
리팩토링 검토 부탁드립니다.
Summary by CodeRabbit
새 기능
접근성 개선