[feature] 데스크탑 상세페이지 지도 클릭 시 전체화면 모달을 추가한다#1440
Conversation
- MapModal에서 데스크탑/모바일별 markerSize, bubbleFontSize, bubbleFontWeight 차등 전달 - InteractiveMapView 말풍선 텍스트 색상을 theme colors.gray[900]로 교체 - bubbleFontWeight prop 추가로 굵기 외부 제어 - React 19 deprecated MutableRefObject → RefObject 교체
mini_mobile 브레이크포인트에서 카드 너비를 calc(100vw - 40px)로 설정
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Warning
|
| Cohort / File(s) | Summary |
|---|---|
문서 frontend/CLAUDE.md |
Naver Map 통합 문서 및 환경 변수 VITE_NAVER_MAP_CLIENT_ID 추가; 에이전트 문서명 변경 (api-hooks-agent.md → API훅부서.md). |
라우팅 / 페이지 삭제 frontend/src/App.tsx, frontend/src/pages/ClubMapPage/ClubMapPage.tsx |
/map 관련 라우트 4개 및 ClubMapPage 파일/컴포넌트 삭제; App에서 해당 import/route 제거. |
새로운 Map UI 컴포넌트 & 스타일 frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsx, .../InteractiveMapView.styles.ts, frontend/src/components/map/MapClubInfoCard/MapClubInfoCard.tsx, .../MapClubInfoCard.styles.ts, frontend/src/components/map/MapModal/MapModal.tsx, .../MapModal.styles.ts, frontend/src/components/map/MapZoomControls/MapZoomControls.tsx, .../MapZoomControls.styles.ts |
InteractiveMapView, MapClubInfoCard, MapModal, MapZoomControls 및 관련 스타일 추가 — 모달 내 지도 렌더링, 오버레이 정보 카드, 확대/축소 UI 포함. |
기존 Map 컴포넌트 변경 frontend/src/components/map/NaverMap/NaverMap.tsx, .../NaverMap.styles.ts |
NaverMap 간단 렌더러 추가/조정 및 MapContainer에 전역 cursor 강제 스타일 적용. |
지도 훅 및 타입 frontend/src/hooks/Map/useNaverMap.ts, frontend/src/hooks/Map/useMapZoom.ts |
useNaverMap 훅 추가(스크립트 로드, 맵/마커 초기화, cleanup, 옵션 포함) 및 useMapZoom 훅 추가(zoomIn/zoomOut). NaverMapInstance 타입과 UseNaverMapOptions 등 선언 추가. |
클럽 상세페이지 통합 변경 frontend/src/pages/ClubDetailPage/ClubDetailPage.tsx, .../ClubDetailPage.styles.ts, frontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.tsx, .../ClubProfileCard.styles.ts |
라우트 기반 지도 링크 제거 및 mapPath→onMapClick 변경으로 MapModal 열기/닫기 제어, 미리보기 카드 클릭 핸들러 추가, NaverMap 사용 방식과 스타일(커서/선택성) 업데이트. |
Sequence Diagram
sequenceDiagram
participant User as "User"
participant ClubDetail as "ClubDetailPage"
participant Modal as "MapModal"
participant View as "InteractiveMapView"
participant NaverAPI as "Naver Map API"
participant Zoom as "MapZoomControls"
User->>ClubDetail: 지도 영역 클릭
ClubDetail->>ClubDetail: isMapModalOpen = true
ClubDetail->>Modal: open(isOpen=true, location, clubName, clubLogo)
Modal->>View: render(active=true, location, mapInstanceRef)
View->>View: useNaverMap(mapRef, lat, lng, options)
View->>NaverAPI: loadNaverMapScript() / naver.maps.Map init
NaverAPI-->>View: map instance (mapInstanceRef에 설정)
View->>View: 마커 및 버블 생성
Modal->>Zoom: useMapZoom(mapInstanceRef) -> 핸들러 전달
User->>Zoom: 클릭(확대/축소)
Zoom->>NaverAPI: setZoom(+1/-1)
User->>Modal: 닫기 버튼 클릭
Modal->>ClubDetail: onClose()
ClubDetail->>ClubDetail: isMapModalOpen = false
View->>NaverAPI: destroy() (cleanup)
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- [feature] 동아리 상세페이지에 동아리방 위치를 지도에 표시한다 #1367: 기존 NaverMap 및 useNaverMap 초기 통합과 밀접한 중복/충돌 가능성.
- [feature] 모바일 상세페이지 지도에 전체화면 전환을 추가한다 #1402: ClubMapPage와 useNaverMap 관련 변경을 다루는 PR로 라우팅/맵 초기화 충돌 가능.
- [feature] 동아리 상세페이지 디자인 개편에 따른 최종 구현 통합 및 API 연동 #972: ClubDetailPage 및 ClubProfileCard의 map 관련 변경과 중복되는 UI/props 수정 가능성.
Suggested reviewers
- oesnuj
- lepitaaar
- seongwon030
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | 제목은 PR의 주요 변경사항인 데스크탑 상세페이지에서 지도 클릭 시 전체화면 모달 추가를 명확하게 설명합니다. |
| Linked Issues check | ✅ Passed | PR은 #1409의 요구사항인 데스크탑 상세페이지 지도 클릭 시 전체화면 모달 기능을 구현했으며, MapModal, InteractiveMapView 등 필요한 컴포넌트와 훅을 모두 추가했습니다. |
| Out of Scope Changes check | ✅ Passed | 모든 변경사항이 지도 모달 기능 구현 및 기존 ClubMapPage 제거와 관련된 범위 내 변경입니다. 불필요한 외부 변경은 없습니다. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ 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
feature/#1409-desktop-detail-map-modal-MOA-803
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.
suhyun113
changed the title
🎨 UI 변경사항을 확인해주세요
9개 스토리 변경 · 전체 56개 스토리 · 22개 컴포넌트 |
![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 (3)
frontend/src/components/map/MapModal/MapModal.tsx (1)
29-29:any타입 사용에 대한 개선 고려
useRef<any>(null)는 코딩 가이드라인에서 권장하지 않는any타입을 사용합니다. Naver Maps API의 타입 정의가 제한적일 수 있지만, 최소한의 인터페이스를 정의하는 것을 고려해 볼 수 있습니다.♻️ 제안된 개선안
+interface NaverMapInstance { + setZoom: (zoom: number) => void; + getZoom: () => number; + setCenter: (latlng: any) => void; + destroy: () => void; +} -const mapInstanceRef = useRef<any>(null); +const mapInstanceRef = useRef<NaverMapInstance | null>(null);
⚠️ 파이프라인에서 Prettier 포맷팅 이슈가 감지되었습니다.npm run format실행을 권장합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/map/MapModal/MapModal.tsx` at line 29, Replace the useRef<any>(null) usage with a properly typed ref: define or import a minimal interface/type for the expected Naver Map instance (or use an existing type like naver.maps.Map if available) and change mapInstanceRef to useRef<YourMapType | null>(null); update any downstream usages (e.g., in initialize or cleanup functions referenced in MapModal) to respect the new type and add narrow casts only where necessary, and finally run npm run format to fix the Prettier formatting issues.frontend/src/pages/ClubDetailPage/ClubDetailPage.styles.ts (1)
66-72: CSS 특이성 충돌 처리가 동작하지만, 더 깔끔한 대안을 고려해볼 수 있습니다.
NaverMap.styles.ts의* { cursor: default !important; }를 오버라이드하기 위해* { cursor: pointer !important; }를 사용하는 것은 동작하지만,!important간의 특이성 충돌은 유지보수 측면에서 취약점이 될 수 있습니다.향후 리팩토링 시 NaverMap 컴포넌트에
interactiveprop을 추가하여 커서 스타일을 내부에서 제어하는 방안을 고려해 볼 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ClubDetailPage/ClubDetailPage.styles.ts` around lines 66 - 72, The current ClubDetailPage.styles.ts uses a global override (* { cursor: pointer !important; }) to beat NaverMap.styles.ts's * { cursor: default !important; }; instead, remove the global * override and control cursor via the NaverMap component by adding an interactive boolean prop (e.g., NaverMap({ interactive })) and using that prop inside NaverMap.styles.ts to set cursor: pointer when interactive and cursor: default when not; then update ClubDetailPage to pass interactive={true} to the NaverMap instance or set cursor on a specific wrapper selector (not via universal *), avoiding !important conflicts and preserving specificity.frontend/src/components/map/MapClubInfoCard/MapClubInfoCard.styles.ts (1)
55-56: 텍스트 선택 차단은 위치 복사 UX를 제한할 수 있습니다.Line 56, Line 82의
user-select: none때문에 사용자가 주소 텍스트를 복사하기 어려워집니다. 특별한 의도가 없다면 제거를 권장합니다.✂️ 제안 diff
export const ClubName = styled.span` ${setTypography(typography.title.title2)}; color: ${colors.base.black}; white-space: nowrap; overflow: hidden; text-overflow: ellipsis; cursor: default; - user-select: none; ${media.tablet} { ${setTypography(typography.title.title5)}; } `; @@ export const LocationText = styled.span` ${setTypography(typography.paragraph.p3)}; color: ${colors.gray[600]}; white-space: nowrap; overflow: hidden; text-overflow: ellipsis; cursor: default; - user-select: none; ${media.tablet} { ${setTypography(typography.paragraph.p6)}; } `;Also applies to: 81-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/map/MapClubInfoCard/MapClubInfoCard.styles.ts` around lines 55 - 56, The CSS rule user-select: none in MapClubInfoCard.styles.ts is preventing users from copying address/text; locate and remove the user-select: none declarations (the ones around the lines referenced and any other occurrences in the MapClubInfoCard styles) or change them to a permissive value like user-select: text for the specific address/text element instead of globally disabling selection; ensure no other selectors in MapClubInfoCard.styles.ts unintentionally block text selection after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/CLAUDE.md`:
- Around line 59-61: Update the CLAUDE.md wording to accurately reflect that
most SDKs are initialized in src/utils/initSDK.ts but Naver Map is dynamically
loaded in src/utils/loadNaverMapScript.ts; change the sentence that currently
states “모든 SDK는 src/utils/initSDK.ts에서 초기화” to separate Naver Map (mentioning
src/utils/loadNaverMapScript.ts) and adjust the similar occurrence around line
74 to match this distinction so readers know Naver Map uses dynamic script
loading.
In `@frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsx`:
- Around line 34-103: The effect currently only clears the timeout but doesn't
cancel the async work started by loadNaverMapScript(), so map initialization can
run after cleanup; fix by introducing a cancelled flag or AbortController inside
the useEffect (e.g., let cancelled = false) and set cancelled = true in the
cleanup, then after loadNaverMapScript().then(...) immediately check if
(cancelled) return before touching mapRef, window.naver, or creating
mapInstanceRef in the then handler; also add a .catch to the promise to
log/ignore errors and ensure mapInstanceRef is only destroyed if it was actually
created (mapInstanceRef.current) to avoid accessing unmounted refs.
In `@frontend/src/components/map/MapModal/MapModal.styles.ts`:
- Around line 17-19: The modal's CSS in MapModal.styles sets height: 100vh which
can be incorrect on mobile; update the height declaration in MapModal.styles
(the styled component or CSS block that currently contains width: 100vw; height:
100vh; max-width: none;) to use height: 100dvh instead so the modal fills the
real viewport height on mobile browsers.
In `@frontend/src/hooks/Map/useMapZoom.ts`:
- Around line 1-4: Replace the loose MutableRefObject<any> with a properly typed
RefObject interface: define an interface (e.g., MapInstance { getZoom(): number;
setZoom(z: number): void }) and change the hook signature from mapInstanceRef:
MutableRefObject<any> to mapInstanceRef: React.RefObject<MapInstance> (React 19
compatible). Update useMapZoom internals (e.g., zoomIn, zoomOut) to null-check
mapInstanceRef.current before calling getZoom()/setZoom() and adjust any usages
to match the new typed methods.
---
Nitpick comments:
In `@frontend/src/components/map/MapClubInfoCard/MapClubInfoCard.styles.ts`:
- Around line 55-56: The CSS rule user-select: none in MapClubInfoCard.styles.ts
is preventing users from copying address/text; locate and remove the
user-select: none declarations (the ones around the lines referenced and any
other occurrences in the MapClubInfoCard styles) or change them to a permissive
value like user-select: text for the specific address/text element instead of
globally disabling selection; ensure no other selectors in
MapClubInfoCard.styles.ts unintentionally block text selection after the change.
In `@frontend/src/components/map/MapModal/MapModal.tsx`:
- Line 29: Replace the useRef<any>(null) usage with a properly typed ref: define
or import a minimal interface/type for the expected Naver Map instance (or use
an existing type like naver.maps.Map if available) and change mapInstanceRef to
useRef<YourMapType | null>(null); update any downstream usages (e.g., in
initialize or cleanup functions referenced in MapModal) to respect the new type
and add narrow casts only where necessary, and finally run npm run format to fix
the Prettier formatting issues.
In `@frontend/src/pages/ClubDetailPage/ClubDetailPage.styles.ts`:
- Around line 66-72: The current ClubDetailPage.styles.ts uses a global override
(* { cursor: pointer !important; }) to beat NaverMap.styles.ts's * { cursor:
default !important; }; instead, remove the global * override and control cursor
via the NaverMap component by adding an interactive boolean prop (e.g.,
NaverMap({ interactive })) and using that prop inside NaverMap.styles.ts to set
cursor: pointer when interactive and cursor: default when not; then update
ClubDetailPage to pass interactive={true} to the NaverMap instance or set cursor
on a specific wrapper selector (not via universal *), avoiding !important
conflicts and preserving specificity.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 472da924-a3c2-49b5-a779-7ac7111d7c9c
⛔ Files ignored due to path filters (1)
frontend/src/assets/images/icons/close_button_icon.svgis excluded by!**/*.svg
📒 Files selected for processing (20)
frontend/CLAUDE.mdfrontend/src/App.tsxfrontend/src/components/map/InteractiveMapView/InteractiveMapView.styles.tsfrontend/src/components/map/InteractiveMapView/InteractiveMapView.tsxfrontend/src/components/map/MapClubInfoCard/MapClubInfoCard.styles.tsfrontend/src/components/map/MapClubInfoCard/MapClubInfoCard.tsxfrontend/src/components/map/MapModal/MapModal.styles.tsfrontend/src/components/map/MapModal/MapModal.tsxfrontend/src/components/map/MapZoomControls/MapZoomControls.styles.tsfrontend/src/components/map/MapZoomControls/MapZoomControls.tsxfrontend/src/components/map/NaverMap/NaverMap.styles.tsfrontend/src/components/map/NaverMap/NaverMap.tsxfrontend/src/hooks/Map/useMapZoom.tsfrontend/src/hooks/Map/useNaverMap.tsfrontend/src/pages/ClubDetailPage/ClubDetailPage.styles.tsfrontend/src/pages/ClubDetailPage/ClubDetailPage.tsxfrontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.styles.tsfrontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.tsxfrontend/src/pages/ClubMapPage/ClubMapPage.tsxfrontend/src/utils/loadNaverMapScript.ts
💤 Files with no reviewable changes (2)
- frontend/src/App.tsx
- frontend/src/pages/ClubMapPage/ClubMapPage.tsx
| - **Naver Map**: 동아리방 위치 지도 (네이버 클라우드 플랫폼) | ||
|
|
||
| 모든 SDK는 `src/utils/initSDK.ts`에서 초기화되며, 각각 환경 변수 필요. |
There was a problem hiding this comment.
Naver Map 초기화 위치 설명이 실제 구현과 다릅니다.
Line 61의 “모든 SDK는 src/utils/initSDK.ts에서 초기화”는 현재 지도 구현과 불일치합니다. Naver Map은 src/utils/loadNaverMapScript.ts에서 동적 로드되므로 문구를 분리해 명시해 주세요.
✍️ 문서 수정 예시
-모든 SDK는 `src/utils/initSDK.ts`에서 초기화되며, 각각 환경 변수 필요.
+Mixpanel/Sentry/Channel.io/Kakao SDK는 `src/utils/initSDK.ts`에서 초기화되며, 각각 환경 변수 필요.
+Naver Map 스크립트는 `src/utils/loadNaverMapScript.ts`에서 동적으로 로드됨.Also applies to: 74-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/CLAUDE.md` around lines 59 - 61, Update the CLAUDE.md wording to
accurately reflect that most SDKs are initialized in src/utils/initSDK.ts but
Naver Map is dynamically loaded in src/utils/loadNaverMapScript.ts; change the
sentence that currently states “모든 SDK는 src/utils/initSDK.ts에서 초기화” to separate
Naver Map (mentioning src/utils/loadNaverMapScript.ts) and adjust the similar
occurrence around line 74 to match this distinction so readers know Naver Map
uses dynamic script loading.
| useEffect(() => { | ||
| if (!active) return; | ||
|
|
||
| const timer = setTimeout(() => { | ||
| loadNaverMapScript().then(() => { | ||
| if (!mapRef.current || !window.naver) return; | ||
|
|
||
| const { naver } = window; | ||
| const position = new naver.maps.LatLng(location.lat, location.lng); | ||
|
|
||
| mapInstanceRef.current = new naver.maps.Map(mapRef.current, { | ||
| center: position, | ||
| zoom: 17, | ||
| logoControl: false, | ||
| mapDataControl: false, | ||
| scaleControl: false, | ||
| }); | ||
|
|
||
| const markerContent = ` | ||
| <div style="position: relative; display: inline-block;"> | ||
| <div style=" | ||
| position: absolute; | ||
| bottom: calc(${markerSize}px + 5px); | ||
| left: 50%; | ||
| transform: translateX(-50%); | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| "> | ||
| <div style=" | ||
| background: #fff; | ||
| border-radius: 50px; | ||
| padding: 10px 16px; | ||
| font-size: ${bubbleFontSize}px; | ||
| font-weight: ${bubbleFontWeight}; | ||
| color: ${colors.gray[900]}; | ||
| white-space: nowrap; | ||
| font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', sans-serif; | ||
| box-shadow: 0 2px 8px rgba(0,0,0,0.15); | ||
| ">동아리방</div> | ||
| <div style=" | ||
| width: 0; | ||
| height: 0; | ||
| border-left: 9px solid transparent; | ||
| border-right: 9px solid transparent; | ||
| border-top: 10px solid #fff; | ||
| margin-top: -2px; | ||
| "></div> | ||
| </div> | ||
| <img src="${markerIcon}" style="width: ${markerSize}px; height: ${markerSize}px; display: block;" /> | ||
| </div> | ||
| `; | ||
|
|
||
| new naver.maps.Marker({ | ||
| position, | ||
| map: mapInstanceRef.current, | ||
| icon: { | ||
| content: markerContent, | ||
| anchor: new naver.maps.Point(markerSize / 2, markerSize), | ||
| }, | ||
| }); | ||
| }); | ||
| }, 100); | ||
|
|
||
| return () => { | ||
| clearTimeout(timer); | ||
| mapInstanceRef.current?.destroy(); | ||
| mapInstanceRef.current = null; | ||
| }; | ||
| }, [active, location.lat, location.lng, markerSize, bubbleFontSize, bubbleFontWeight]); |
There was a problem hiding this comment.
비동기 작업의 cleanup 처리 개선 필요
clearTimeout(timer)은 타이머만 취소하지만, 이미 실행된 loadNaverMapScript().then() 체인 내부의 로직은 취소되지 않습니다. 모달이 빠르게 열고 닫힐 경우, cleanup 이후에도 맵 초기화가 진행되어 이미 언마운트된 ref에 접근할 수 있습니다.
🛡️ 제안된 수정안
useEffect(() => {
if (!active) return;
+ let isCancelled = false;
const timer = setTimeout(() => {
loadNaverMapScript().then(() => {
+ if (isCancelled) return;
if (!mapRef.current || !window.naver) return;
const { naver } = window;
const position = new naver.maps.LatLng(location.lat, location.lng);
// ... rest of initialization
});
}, 100);
return () => {
+ isCancelled = true;
clearTimeout(timer);
mapInstanceRef.current?.destroy();
mapInstanceRef.current = null;
};
}, [active, location.lat, location.lng, markerSize, bubbleFontSize, bubbleFontWeight]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsx` around
lines 34 - 103, The effect currently only clears the timeout but doesn't cancel
the async work started by loadNaverMapScript(), so map initialization can run
after cleanup; fix by introducing a cancelled flag or AbortController inside the
useEffect (e.g., let cancelled = false) and set cancelled = true in the cleanup,
then after loadNaverMapScript().then(...) immediately check if (cancelled)
return before touching mapRef, window.naver, or creating mapInstanceRef in the
then handler; also add a .catch to the promise to log/ignore errors and ensure
mapInstanceRef is only destroyed if it was actually created
(mapInstanceRef.current) to avoid accessing unmounted refs.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
Reviewing the architectural changes and implementation details.
Summary of findings:
- Architectural Improvement: Good job on consolidating the map view into a responsive modal. The component breakdown and hook separation are well-structured.
- Memory Management: Explicit cleanup of the Naver Map instance is a great addition to prevent memory leaks.
- CI Status: The Frontend CI is currently failing due to Prettier formatting issues in 5 files. Please run to fix these.
- Typing: There are several instances of types (especially in refs). Using proper interfaces for the Naver Map instance would improve type safety.
- Async Safety: The in should handle component unmounting during the async script loading to avoid potential issues.
I will add specific inline comments for these points.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.tsx (1)
113-124:⚠️ Potential issue | 🔴 Critical
Styled.MapLink요소의 키보드 접근성을 개선해야 합니다.
Styled.MapLink가styled.span으로 정의되어 있어onClick이벤트만으로는 키보드 사용자가 접근할 수 없습니다. 다음 중 하나로 수정하세요:
styled.button으로 변경하거나role="button",tabIndex={0},onKeyDown핸들러를 추가하여 Enter/Space 키 지원현재 상태에서는 TAB 네비게이션 및 스크린 리더 사용자에게 상호작용 요소로 인식되지 않습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.tsx` around lines 113 - 124, The Map link currently uses Styled.MapLink (a styled.span) with only onClick, which is not keyboard-accessible; update the implementation so Keyboard users can activate it: either redefine Styled.MapLink as a styled.button (preferred) or add accessibility props to the existing styled.span — role="button", tabIndex={0} and an onKeyDown handler on Styled.MapLink that calls onMapClick when Enter or Space is pressed; ensure onMapClick is used for both click and key activation and that visual focus styles remain visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/map/MapModal/MapModal.tsx`:
- Around line 29-30: Replace the implicit any on mapInstanceRef with a concrete
interface (e.g., ZoomableMap) that at minimum exposes getZoom and setZoom, and
update the related types in useMapZoom and InteractiveMapView to use that
interface; create/export ZoomableMap from a shared file (e.g.,
hooks/Map/useMapZoom or a common types file), change const mapInstanceRef =
useRef<any>(null) to useRef<ZoomableMap | null>(null), and update
function/method signatures in useMapZoom and InteractiveMapView to accept/return
ZoomableMap so TypeScript can catch API typos and enforce correct usage.
In `@frontend/src/pages/ClubDetailPage/ClubDetailPage.tsx`:
- Line 180: Styled.MapCard is currently a styled.div with only an onClick
handler so it isn't keyboard-accessible; update the element to support keyboard
interaction by either converting Styled.MapCard into a semantic button element
or adding accessibility attributes and handlers: give it role="button",
tabIndex={0}, and implement an onKeyDown handler that calls
setIsMapModalOpen(true) when Enter or Space is pressed (mirror the pattern used
in ShareButton for consistent behavior and focus handling). Ensure focus styles
are preserved and that the change references Styled.MapCard and the
setIsMapModalOpen call so keyboard users can open the map modal.
---
Outside diff comments:
In
`@frontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.tsx`:
- Around line 113-124: The Map link currently uses Styled.MapLink (a
styled.span) with only onClick, which is not keyboard-accessible; update the
implementation so Keyboard users can activate it: either redefine Styled.MapLink
as a styled.button (preferred) or add accessibility props to the existing
styled.span — role="button", tabIndex={0} and an onKeyDown handler on
Styled.MapLink that calls onMapClick when Enter or Space is pressed; ensure
onMapClick is used for both click and key activation and that visual focus
styles remain visible.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 99ee5514-fc1d-41ac-ad20-2c9652b66099
📒 Files selected for processing (5)
frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsxfrontend/src/components/map/MapModal/MapModal.styles.tsfrontend/src/components/map/MapModal/MapModal.tsxfrontend/src/pages/ClubDetailPage/ClubDetailPage.tsxfrontend/src/pages/ClubDetailPage/components/ClubProfileCard/ClubProfileCard.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/map/MapModal/MapModal.styles.ts
- frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsx
| {clubLocation && ( | ||
| <Styled.MapInfo> | ||
| <Styled.MapCard> | ||
| <Styled.MapCard onClick={() => setIsMapModalOpen(true)}> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# MapCard의 실제 styled base element와 키보드 처리 유무 확인
fd -i "ClubDetailPage.styles.ts*" frontend/src/pages/ClubDetailPage
rg -n -C4 "MapCard" frontend/src/pages/ClubDetailPage
rg -n -C2 "onKeyDown|tabIndex|role=['\"]button['\"]" frontend/src/pages/ClubDetailPageRepository: Moadong/moadong
Length of output: 2963
지도 카드의 접근성 개선 필요
Styled.MapCard가 styled.div로 정의되어 있으면서 onClick 핸들러만 있고 키보드 입력을 지원하지 않습니다. 동일 파일의 ShareButton 컴포넌트처럼 role="button", tabIndex={0}, onKeyDown (Enter/Space 처리)을 추가하거나, 의미 있는 <button> 요소로 변경해주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/pages/ClubDetailPage/ClubDetailPage.tsx` at line 180,
Styled.MapCard is currently a styled.div with only an onClick handler so it
isn't keyboard-accessible; update the element to support keyboard interaction by
either converting Styled.MapCard into a semantic button element or adding
accessibility attributes and handlers: give it role="button", tabIndex={0}, and
implement an onKeyDown handler that calls setIsMapModalOpen(true) when Enter or
Space is pressed (mirror the pattern used in ShareButton for consistent behavior
and focus handling). Ensure focus styles are preserved and that the change
references Styled.MapCard and the setIsMapModalOpen call so keyboard users can
open the map modal.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
전체적으로 지도 기능을 모달로 분리하고 컴포넌트화한 점이 매우 좋습니다. 코드 구조가 훨씬 깔끔해졌네요.
다만, loadNaverMapScript.ts 에서 잠재적인 Race Condition이 발견되었습니다. existingScript가 존재하더라도 아직 로드가 완료되지 않은 상태(window.naver 미정의)일 수 있습니다. 이 경우 즉시 resolve()를 호출하면 지도가 정상적으로 초기화되지 않을 수 있으니, 기존 스크립트가 있더라도 로드 완료 이벤트를 기다리도록 보강이 필요해 보입니다.
또한 프로젝트 정책상 ESLint warning이 0이어야 머지가 가능하므로, 새로 추가된 파일들에서 발생할 수 있는 lint 경고가 없는지 확인 부탁드립니다.
그 외 메모리 해제(destroy) 로직이나 반응형 대응은 잘 구현되었습니다. 위 사항 검토 부탁드립니다.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/hooks/Map/useNaverMap.ts`:
- Around line 79-125: The effect has a race where cleanup can run before
loadNaverMapScript() resolves; fix by adding an abort flag inside useEffect
(e.g., let aborted = false) and set aborted = true in the cleanup, then after
loadNaverMapScript().then(...) but before creating/assigning mapInstance check
if (aborted) return; (or if you create mapInstance still check aborted and
immediately destroy it and null externalRef). Update references in this effect:
useEffect, loadNaverMapScript, mapInstance, mapRef, externalRef and ensure
cleanup still calls mapInstance?.destroy() and clears externalRef.current.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5c62b656-a700-4be5-b467-fa5ced64f131
📒 Files selected for processing (8)
frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsxfrontend/src/components/map/MapModal/MapModal.styles.tsfrontend/src/components/map/MapModal/MapModal.tsxfrontend/src/components/map/MapZoomControls/MapZoomControls.styles.tsfrontend/src/components/map/NaverMap/NaverMap.tsxfrontend/src/hooks/Map/useMapZoom.tsfrontend/src/hooks/Map/useNaverMap.tsfrontend/src/pages/ClubDetailPage/ClubDetailPage.tsx
✅ Files skipped from review due to trivial changes (2)
- frontend/src/components/map/MapZoomControls/MapZoomControls.styles.ts
- frontend/src/components/map/MapModal/MapModal.styles.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/src/hooks/Map/useMapZoom.ts
- frontend/src/components/map/NaverMap/NaverMap.tsx
- frontend/src/components/map/InteractiveMapView/InteractiveMapView.tsx
- frontend/src/components/map/MapModal/MapModal.tsx
| useEffect(() => { | ||
| if (!active) return; | ||
|
|
||
| let mapInstance: NaverMapInstance | null = null; | ||
|
|
||
| loadNaverMapScript().then(() => { | ||
| if (!mapRef.current || !window.naver) return; | ||
|
|
||
| const { naver } = window; | ||
| const position = new naver.maps.LatLng(lat, lng); | ||
|
|
||
| mapInstance = new naver.maps.Map(mapRef.current, { | ||
| center: position, | ||
| zoom: 17, | ||
| logoControl: false, | ||
| mapDataControl: false, | ||
| scaleControl: false, | ||
| draggable: interactive, | ||
| scrollWheel: interactive, | ||
| keyboardShortcuts: interactive, | ||
| disableDoubleClickZoom: !interactive, | ||
| pinchZoom: interactive, | ||
| }); | ||
|
|
||
| if (externalRef) { | ||
| externalRef.current = mapInstance; | ||
| } | ||
|
|
||
| new naver.maps.Marker({ | ||
| position, | ||
| map: mapInstance, | ||
| icon: { | ||
| content: buildMarkerContent( | ||
| markerSize, | ||
| bubbleText, | ||
| bubbleFontSize, | ||
| bubbleFontWeight, | ||
| ), | ||
| anchor: new naver.maps.Point(markerSize / 2, markerSize), | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| return () => { | ||
| mapInstance?.destroy(); | ||
| if (externalRef) externalRef.current = null; | ||
| }; |
There was a problem hiding this comment.
비동기 초기화와 cleanup 사이의 race condition 가능성
loadNaverMapScript().then() 내부에서 지도가 생성되기 전에 cleanup이 실행될 수 있습니다. active가 false로 변경되거나 컴포넌트가 언마운트되면 cleanup이 먼저 실행되고, 이후 Promise가 resolve되어 지도가 생성될 수 있습니다. 이 경우 생성된 지도 인스턴스가 정리되지 않아 메모리 누수가 발생합니다.
🛡️ abort flag를 사용한 race condition 방지
useEffect(() => {
if (!active) return;
let mapInstance: NaverMapInstance | null = null;
+ let aborted = false;
loadNaverMapScript().then(() => {
+ if (aborted) return;
if (!mapRef.current || !window.naver) return;
const { naver } = window;
const position = new naver.maps.LatLng(lat, lng);
mapInstance = new naver.maps.Map(mapRef.current, {
// ... options
});
if (externalRef) {
externalRef.current = mapInstance;
}
new naver.maps.Marker({
// ... marker config
});
});
return () => {
+ aborted = true;
mapInstance?.destroy();
if (externalRef) externalRef.current = null;
};
}, [/* deps */]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/Map/useNaverMap.ts` around lines 79 - 125, The effect has
a race where cleanup can run before loadNaverMapScript() resolves; fix by adding
an abort flag inside useEffect (e.g., let aborted = false) and set aborted =
true in the cleanup, then after loadNaverMapScript().then(...) but before
creating/assigning mapInstance check if (aborted) return; (or if you create
mapInstance still check aborted and immediately destroy it and null
externalRef). Update references in this effect: useEffect, loadNaverMapScript,
mapInstance, mapRef, externalRef and ensure cleanup still calls
mapInstance?.destroy() and clears externalRef.current.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
전체적으로 지도 기능을 모달로 분리하고 컴포넌트화한 점이 매우 좋습니다. 코드 구조가 훨씬 깔끔해졌네요.
다만, 다음 사항들에 대해 보완이 필요해 보입니다:
거절 사유(비차단 아님)
-
Severity: Medium
-
항목: loadNaverMapScript.ts의 Race Condition
-
영향: 기존 스크립트 태그가 존재하지만 아직 로드가 완료되지 않은 상태(window.naver 미정의)에서 즉시 resolve()가 호출되어, 지도가 정상적으로 초기화되지 않을 수 있음.
-
수정방안: existingScript 분기에서도 load/error 리스너를 연결하여 로드 완료를 보장한 뒤 resolve/reject 처리.
-
Severity: Low
-
항목: ESLint max-warnings=0 위반 가능성
-
영향: 프로젝트 정책상 경고가 없어야 머지가 가능함. 새로 추가된 파일들(InteractiveMapView.tsx 등)에서 미사용 변수나 import가 없는지 확인 필요.
위 사항들 검토 부탁드립니다.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
Architecture & UX Review
- Unified Logic: Excellent consolidation of mobile/desktop map views into a single responsive MapModal.
- Refactoring: Strong decoupling of business logic from UI via useNaverMap and useMapZoom hooks.
- Performance/Safety: Good catch on the mapInstance.destroy() cleanup to prevent memory leaks and the use of 100dvh for mobile viewport accuracy.
- Type Safety: Removal of any and introduction of NaverMapInstance interface significantly improves the DX.
Suggestion: Regarding your discussion point on /webview/club/:clubId/map, since this was an internal route for the webview, removing it is correct for a unified experience, provided the native bridge (requestNavigateBack) is still compatible with the new modal-based flow.
LGTM.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
Refactoring map functionality into a responsive modal is a significant improvement. I've confirmed that the race condition in loadNaverMapScript.ts and the previous any types have been addressed through centralized hooks and better type definitions. The cleanup of the unused map route also simplifies the application structure.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
전체적으로 지도 기능을 모달로 분리하고 컴포넌트화한 점이 매우 좋습니다. 코드 구조가 훨씬 깔끔해졌네요.
다만, loadNaverMapScript.ts 에서 잠재적인 Race Condition이 발견되었습니다. existingScript가 존재하더라도 아직 로드가 완료되지 않은 상태(window.naver 미정의)일 수 있습니다. 이 경우 즉시 resolve()를 호출하면 지도가 정상적으로 초기화되지 않을 수 있으니, 기존 스크립트가 있더라도 로드 완료 이벤트를 기다리도록 보강이 필요해 보입니다.
또한 프로젝트 정책상 ESLint warning이 0이어야 머지가 가능하므로, 새로 추가된 파일들에서 발생할 수 있는 lint 경고가 없는지 확인 부탁드립니다.
그 외 메모리 해제(destroy) 로직이나 반응형 대응은 잘 구현되었습니다. 위 사항 검토 부탁드립니다.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
LGTM. 별도 페이지로 분리되어 있던 지도를 로 통합하여 사용자 경험을 개선하고, 네이버 지도 관련 로직을 컴포넌트화하여 재사용성을 높인 점이 매우 좋습니다. 특히 반응형 디자인과 줌 컨트롤 추가 등 디테일한 부분까지 잘 고려되었습니다.
lepitaaar
left a comment
lepitaaar
left a comment
There was a problem hiding this comment.
LGTM. 별도 페이지로 분리되어 있던 지도를 MapModal로 통합하여 사용자 경험을 개선하고, 네이버 지도 관련 로직을 컴포넌트화하여 재사용성을 높인 점이 매우 좋습니다. 특히 반응형 디자인과 줌 컨트롤 추가 등 디테일한 부분까지 잘 고려되었습니다.
| const handleRecenter = useCallback(() => { | ||
| const map = mapInstanceRef.current; |
There was a problem hiding this comment.
Recenter 중심으로 돌아간다는 의미인가요??
There was a problem hiding this comment.
네, 사용자가 지도를 드래그하여 이동한 뒤 하단 MapClubInfoCard를 클릭하면 원래 동아리 위치 기준으로 지도 중심을 되돌리는 기능입니다!
| position: absolute; | ||
| bottom: 50px; | ||
| right: 40px; | ||
| z-index: 10; |
There was a problem hiding this comment.
z-index를 말씀하시는거면, 기존 Z_INDEX는 전역 레이어용으로 만든 거라 같은 페이지 내에서만 쓰이는 z-index: 10은 따로 분리하지 않았습니다.
두 번뿐이라 고민했는데, 매직넘버에 이름을 붙이는 효과가 있으니 파일 내 로컬 상수(CONTROL_Z_INDEX)로 분리해 반영하겠습니다. fdfe0f3
| } | ||
|
|
||
| export const useMapZoom = ( | ||
| mapInstanceRef: RefObject<NaverMapInstance | null>, |
There was a problem hiding this comment.
MutableRefObject라는 타입도 있는데
리액트 19부터 RefObject나 MutableRefObject나 기능이 같아졌다고 하네요.
There was a problem hiding this comment.
맞아요! 두 타입이 React 19부터 기능적으로 동일해졌더라고요. 저도 처음에 MutableRefObject로 작성했다가 IDE에서 취소선이 떠서 확인해보니 deprecated 처리된 걸 알게 됐고, mutable 동작이 RefObject로 통합되면서 권장 타입이 됐다고 해서 그쪽으로 통일했습니다.
existingScript가 있어도 로딩 중일 수 있어 window.naver가 undefined인 상태로 resolve()될 수 있었네요. lint를 확인했는데 전체 57건의 warning이 발견되었습니다. 이번 PR로 추가한 지도 관련 신규 파일에서는 경고가 발생하지 않았기 때문에, 별도의 브랜치에서 정리하는 방향으로 진행하는게 좋을 것 같습니다. |
3 participants
#️⃣연관된 이슈
close #1409
📝작업 내용
기존 모바일 전용 전체화면 지도 라우트(
ClubMapPage,/clubDetail/:clubId/map외)를 제거하고, 데스크탑 / 모바일에서 서로 다르게 보이는 반응형 모달(MapModal) 로 통합했습니다.🗑️ 제거
src/pages/ClubMapPage/전체 삭제/clubDetail/:clubId/map,/clubDetail/@:clubName/map,/webview/club/:clubId/map,/webview/club/@:clubName/map라우트 제거✨ 신규/개편
MapModal—PortalModal기반 반응형 모달.useDevice로 모바일/데스크탑 분기100dvh) + 좌상단 뒤로가기 버튼 + 핀치줌MapZoomControls줌 인/아웃 버튼 (모바일에선 아예 렌더 안 함)InteractiveMapView— 모달 내부에서 쓰이는 인터랙션 지도 공통 뷰. 마커/말풍선 크기·폰트가 디바이스별로 달라짐activeprop으로 닫힌 모달에서는 지도 초기화 skipmap.setCenter()로 초기 위치 복귀mapInstance.destroy()호출 (메모리 누수 방지)MapClubInfoCard— 기존ClubMapPageBottomCard를 공용 카드로 분리, mini_mobile 반응형 너비 대응MapZoomControls+useMapZoom— 데스크탑용 줌 버튼 UI/로직 분리 (NaverMapInstance인터페이스 공유)🧹 리팩토링 / 정리
components/map/{NaverMap,InteractiveMapView,MapModal,MapClubInfoCard,MapZoomControls}+hooks/Map/{useNaverMap,useMapZoom}+utils/loadNaverMapScriptuseNaverMap훅 확장으로 중복 로직 통합 — 기존엔useNaverMap(미리보기)과InteractiveMapView(모달) 각각이 지도 초기화 로직을 별도로 들고 있었음.active/markerSize/bubbleText/bubbleFontSize/bubbleFontWeight/mapInstanceRef옵션을 추가해 훅 하나로 일원화,InteractiveMapView는 훅 호출 + JSX만 담당하는 얇은 래퍼로 축소buildMarkerContent헬퍼로 통합 — 기존 두 파일에 거의 동일하게 있던 약 35줄 템플릿을 한 곳으로any타입 전면 제거 —NaverMapInstance인터페이스(useMapZoom에 정의) 도입으로MapModal,InteractiveMapView,useMapZoom,useNaverMap에서 지도 인스턴스 타입 명시NaverMapprops에Pick<ClubLocation, 'lat' | 'lng'>적용 —ClubProfileCard패턴 일관성MapZoomControls조건부 렌더로 전환 — 기존 CSSdisplay:none방식 제거, 불필요한 DOM/훅 호출 제거InteractiveMapView의internalMapRef폴백 제거 —mapInstanceRef를 필수 prop으로 승격colors.gray[900])로 통일🐛 부수 수정
100vh→100dvh(모바일 주소바 영역 대응)useNaverMapuseEffect 의존성 배열 누락 수정중점적으로 리뷰받고 싶은 부분(선택)
논의하고 싶은 부분(선택)
/webview/club/:clubId/map)를 제거했는데, 네이티브 앱 쪽에서 해당 경로를 참조하는 곳이 없는지 확인이 필요합니다.🫡 참고사항
Summary by CodeRabbit
새로운 기능
UI/UX 개선
문서