fix: 라우트 변경 시 모달, 알림 패널 등 오버레이 닫힘 처리 공통화 - #78
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 (4)
📝 WalkthroughWalkthrough공통 Changes경로 변경 시 패널 자동 닫기
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Router as Next.js pathname
participant Hook as useCloseOnPathnameChange
participant Header
participant NotificationTrigger
participant ModalMain
Router->>Hook: pathname 변경 전달
Hook->>Header: closeSideNav 실행
Hook->>NotificationTrigger: closeQuiet 실행
Hook->>ModalMain: onClose 실행
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
juengseulki
left a comment
juengseulki
left a comment
There was a problem hiding this comment.
전체 변경사항 확인했습니다!
👍 좋았던 점
- 페이지 이동, 뒤로가기, 앞으로가기 시 이전 화면의 Overlay가 남는 문제를 공통 Hook으로 정리했습니다.
- 최초 마운트에서는 닫기 callback을 실행하지 않고, pathname이 실제로 변경된 경우에만 실행하도록 구성했습니다.
- 최신
onClosecallback을 Ref로 관리해 stale closure를 방지했습니다. - callback 참조 변경 때문에 pathname 감지 Effect가 불필요하게 다시 실행되지 않도록 했습니다.
- 같은 pathname에서 Query String이나 화면 상태만 변경되는 경우에는 Overlay를 유지하는 정책을 명확히 적용했습니다.
- 공통 Modal에 Hook을 적용해 로그인 안내 등 모든 공통 Modal 사용처가 자동으로 같은 정책을 따르도록 했습니다.
- Header 사이드바와 알림 패널에도 동일 Hook을 적용해 Overlay별 개별 구현을 제거했습니다.
- Header에서 렌더 도중 pathname을 비교하며 상태를 변경하던 기존 로직을 Effect 기반 처리로 교체했습니다.
- 사이드바와 알림 패널의 기존 닫기 callback을 그대로 재사용해 닫기 책임이 중복되지 않았습니다.
- pathname Ref를 갱신한 뒤 callback을 실행해 callback 과정의 상태 변경에도 비교적 안정적으로 동작하도록 구성했습니다.
- 변경 파일이 4개로 제한되어 있고, 공통 Hook 추가와 적용 범위가 명확합니다.
- 현재 PR은 열려 있고 병합 가능한 상태입니다.
🔍 확인 및 제안
최신 onClose를 Ref로 관리하는 방식은 적절합니다.
onClose를 pathname 감지 Effect의 의존성에 직접 포함하면
부모 렌더링으로 callback 참조가 바뀔 때마다 Effect가 다시 평가되고,
구현 방식에 따라 실제 경로 변경이 아닌데도 닫기 로직과 결합될 가능성이 있습니다.
현재 구현은 다음처럼 역할이 잘 분리되어 있습니다.
- 첫 번째 Effect: 최신
onClose저장 - 두 번째 Effect: pathname 변경만 감지
- pathname 변경 시: 최신 callback 실행
따라서 경로 감지와 callback 최신화가 서로 독립적으로 동작합니다.
현재 기준은 pathname이므로 Query String이나 같은 페이지 내부 상태 변경에는 Overlay가 유지됩니다.
PR 설명의 정책과 일치합니다.
다만 Hash 변경도 화면 이동으로 간주해야 하는 요구가 생기면
현재 Hook만으로는 감지되지 않으므로 후속으로 별도 기준을 검토하면 좋겠습니다.
전체적으로 반복되던 닫기 처리를 공통화하면서
기존 Overlay별 닫기 로직은 재사용한 깔끔한 변경으로 보입니다.
수고하셨습니다! 😊
To Reviewer 내용 기준으로 pathname 변경 시점과
최신 onClose callback을 Ref로 관리하는 방식을 중점적으로 확인했습니다!
현재 Hook은 usePathname()의 결과만 감지하기 때문에
실제 pathname이 변경될 때만 닫기 callback이 실행됩니다.
따라서 다음 동작에서는 Overlay가 닫힙니다.
- 다른 페이지 링크로 이동
router.push()또는router.replace()로 pathname 변경- 브라우저 뒤로가기
- 브라우저 앞으로가기
반대로 다음과 같은 동일 페이지 내부 변경에서는 유지됩니다.
- Query String 변경
- 필터·정렬 상태 변경
- 컴포넌트 로컬 상태 변경
- React Query 데이터 갱신
PR에 작성된 동작 정책과 일치하는 것으로 확인했습니다.
최신 onClose callback을 Ref로 관리한 방식도 적절합니다.
현재 구현은 callback 최신화와 pathname 감지를 서로 다른 Effect로 분리합니다.
onClose가 바뀌면onCloseRef.current만 갱신- pathname이 바뀌면
onCloseRef.current에 저장된 최신 함수 실행
이 구조 덕분에 pathname 감지 Effect가
onClose 함수 참조 변화에 영향을 받지 않으면서도
실제 닫기 시점에는 최신 callback을 사용할 수 있습니다.
최초 마운트에서는 초기 pathname과 현재 pathname이 같으므로
닫기 callback이 호출되지 않고,
첫 번째 실제 경로 변경부터 실행되는 흐름도 적절합니다.
공통 Modal에 적용한 범위도 자연스럽습니다.
로그인 Modal을 포함한 공통 Modal 사용처가 모두 같은 동작을 따르고,
onClose가 없는 Modal은 Optional Chaining으로 아무 동작도 하지 않아 기존 계약을 유지합니다.
Header 사이드바의 기존 렌더 중 상태 변경 로직을 제거하고
동일 Hook을 사용하도록 변경한 부분도 더 안전한 React 패턴으로 보입니다.
알림 패널 역시 기존의 closeQuiet callback을 재사용해
바깥 클릭과 라우트 변경이 동일한 닫기 처리를 공유합니다.
|
@juengseulki
|
2 participants
📋 작업 내용
페이지 이동·뒤로가기·앞으로가기 시 이전 화면의 모달과 헤더 오버레이가 남지 않도록 닫힘 처리를 공통화했습니다.
🔥 변경 사항
useCloseOnPathnameChange공통 hook: 최초 마운트는 유지하고 pathname 변경 시 최신 닫기 콜백을 실행Modal에 훅을 적용해 로그인 모달을 포함한 공통 모달이 라우트 이동 시 닫히도록 수정✅ 체크리스트
📷 스크린샷 (선택)
UI 변경 없음
🔗 관련 이슈
관련 이슈 없음
💬 To Reviewer
onClose콜백을 ref로 관리하는 방식이 적절한지 중점적으로 확인 부탁드립니다.Summary by CodeRabbit