Repository navigation
Conversation
198c73c to
6ad4994
Compare
|
수고하셨습니다! PR 확인해볼게요~ |
dennis0405
left a comment
dennis0405
left a comment
There was a problem hiding this comment.
공유 링크로 모바일 웹에 들어온 사용자를 앱으로 연결하는 방향과 경로 변환 로직을 순수 함수로 분리한 점은 좋았습니다. 다만 현재는 아래 두 항목을 머지 전에 정리해야 한다고 판단했습니다.
- 기존 하단 CTA가 남아 있어 동아리 상세에서는 상단 배너와 앱 유도 UI가 중복됩니다. 이 때문에 배너의 7일 닫기, 데스크톱 미노출, React Native WebView 미노출 정책도 하단 CTA에는 적용되지 않습니다.
- 배너 닫기 시 사용하는 localStorage 쓰기 경로가 예외를 안전하게 처리하지 않아, 스토리지 접근이 차단되거나 quota 오류가 발생하면 이벤트 핸들러에서 예외가 날 수 있습니다.
추가로 AppBannerProvider 수준의 테스트를 보완해 모바일/데스크톱/WebView 노출 조건, 7일 닫기와 만료, 스토리지 사용 불가 상황을 검증해 주세요. 현재 배너는 모든 /club 경로에 표시되지만 동아리 상세 외 경로는 앱 홈으로 이동하므로, 이 동작이 제품 의도인지도 확인 부탁드립니다.
검증 결과 웹 typecheck와 전체 웹 테스트 71개가 통과했고, PR CI 4개도 모두 성공 상태임을 확인했습니다.
| </main> | ||
|
|
||
| {/* 하단 고정 앱 유도 CTA */} | ||
| {/* 하단 고정 앱 유도 CTA (웹 전용) */} |
There was a problem hiding this comment.
기존 하단 CTA가 그대로 남아 있어 동아리 상세에서는 새 상단 배너와 앱 유도 UI가 중복됩니다. 상단 배너를 닫아도 이 CTA는 계속 보이고, 데스크톱이나 React Native WebView에서도 이 CTA에는 배너의 노출 제한이 적용되지 않습니다. 이번 변경이 기존 CTA를 상단으로 옮기는 것이라면 이 하단 CTA를 제거해 주세요. 두 위치를 의도적으로 병행한다면 닫기 및 플랫폼별 노출 정책을 명확히 맞춰야 합니다.
| GA.setEvent({ category: 'AppBanner', action: 'dismiss', label: platform ?? '' }) | ||
| // 스토리지 쓰기가 막힌 환경(사파리 시크릿 모드 등)에서도 배너는 닫히도록 상태부터 바꾼다 | ||
| setIsDismissed(true) | ||
| LocalStorage.setWithExpiry(APP_BANNER_DISMISSED_STORAGE_KEY, 'true', DISMISS_DURATION_MS) |
There was a problem hiding this comment.
바로 위 주석과 달리 LocalStorage.setWithExpiry() 내부의 localStorage.setItem()은 try/catch로 보호되지 않습니다. 스토리지 접근 차단이나 quota 오류가 발생하면 닫기 핸들러에서 예외가 발생할 수 있고, getWithExpiry()도 catch 내부의 removeItem()이 다시 실패할 수 있습니다. 저장 실패와 무관하게 현재 세션에서는 정상적으로 닫히도록 스토리지 get/set/remove를 안전하게 감싸고, 사용 불가·손상된 값·7일 만료 케이스를 테스트해 주세요. 또한 이 키 하나로 모든 /club 경로에서 7일간 숨기는 정책이 의도된 것인지도 확인 부탁드립니다.
|
오늘 다시 한번 확인하겠습니다. 일단 지금까지의 작업은 Fable로 초안을 만들어본 거고, 디스코드에도 조만간 글을 올려볼게요. |
dennis0405
left a comment
dennis0405
left a comment
There was a problem hiding this comment.
PR 본문 보강을 부탁드립니다. 현재 본문에는 상단 앱 이동 버튼 추가와 모바일 디자인 교체만 적혀 있지만, 실제 diff는 161개 파일(+6,228/-3,105)로 다음과 같은 전반적인 웹 앱 변경까지 포함하고 있습니다.
- 동아리 상세·모집공고·리뷰 화면과 기능 확장
- 검색·저장·마이페이지·운영진 관리 플로우 변경
- 인증 및 API 모듈 재구성
- 공통 컴포넌트·디렉터리 구조 리팩터링
- 기존 home/index 및 career 관련 코드 삭제
- 로고·파비콘 등 에셋 교체
리뷰어가 의도와 회귀 범위를 판단할 수 있도록 주요 기능별 변경 내용, 삭제·마이그레이션 항목, 영향받는 사용자 플로우, 실제로 수행한 테스트를 PR 본문에 구체적으로 정리해 주세요. 공유/앱 배너 변경과 웹 전면 리뉴얼을 별도 PR로 분리할 수 있다면 그 편이 가장 좋고, 하나의 PR로 유지한다면 적어도 위 범위를 본문에서 모두 설명해야 합니다.
| // UA와 스토리지는 브라우저에서만 알 수 있어 첫 렌더 이후에 판단한다 | ||
| useEffect(() => { | ||
| // 앱 웹뷰로 열린 페이지(약관 등)에서는 앱을 다시 권할 이유가 없다 | ||
| if (window.ReactNativeWebView) { |
There was a problem hiding this comment.
Consider: 데스크톱과 React Native WebView에서 상단 배너를 제외하는 조건 자체는 적절해 보이지만, 현재 이 동작을 보장하는 컴포넌트 테스트가 없습니다. 모바일 UA에서는 노출, 데스크톱 UA와 window.ReactNativeWebView 환경에서는 미노출, /club/auth에서는 미노출되는 케이스를 AppBannerProvider 수준에서 검증해 주세요. 또한 iPadOS 13+ Safari의 기본 데스크톱 UA는 현재 배너 대상에서 제외되는데, 태블릿도 모바일 유입으로 볼 것인지 제품 정책 확인이 필요합니다. 앞선 코멘트처럼 하단 CTA를 유지한다면 이 제외 정책은 상단 배너에만 적용된다는 점도 함께 정리되어야 합니다.
| */ | ||
| export function toAppDeepPath(pathname: string): string { | ||
| const matched = CLUB_DETAIL_PATHNAME.exec(pathname) | ||
| return matched ? `club/${matched[1]}` : '' |
There was a problem hiding this comment.
Consider: 배너는 모든 /club 하위 페이지에 노출되지만 현재 화면을 앱에서 이어서 여는 것은 동아리 상세뿐이고, /club/search, /club/saved, /club/mypage 등에서는 앱 홈으로 이동합니다. 사용자는 배너를 현재 화면을 앱에서 이어서 보는 기능으로 이해할 수 있으므로, 해당 화면들의 딥링크를 추가하거나 배너 노출을 실제로 대응되는 경로로 제한하는 방안을 검토해 주세요. 앱 홈 이동이 의도된 정책이라면 UI 문구와 PR 본문에 그 제한을 명시하는 편이 좋겠습니다.
|
일단 AI 통해서 간단한 리뷰들 정도 남겨봤는데, |
|
수고하셨습니다 |
|
이 브랜치가 web 앱을 갱신하다가 리팩토링으로 방향을 틀은 셈인데 |
작업 내용
변경 이유
지금 각 동아리 페이지에서 공유 버튼을 누르면 링크 텍스트를 전달할 수 있게 되어 있습니다.
그래서 지금 이 경로로 들어갈 수 있게 되어 있습니다.
문제는 해당 web app의 상단에 올클리어 앱으로 보기 기능이 없어서 웹에 갇히게 될 수도 있다는 점이었습니다.
중요한 작업은 아니라서 일단은 develop -> main 브랜치 전환 release: v3.0.1 (develop → main) #159 작업이 마무리 되면 그 다음에 진행하면 될것 같습니다
테스트한 내용
스크린샷 (선택)
체크