Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI 코드 리뷰 - 버그 관점
프로필 이미지를 로컬 에셋(URL/UIImage)에서 Firebase Storage 기반 비동기 로딩으로 전환하는 PR이다. User.profileImage 타입이 URL에서 String(식별자)으로 변경되었고, StorageProfileImageRepository와 UIImageView+ProfileImage extension이 새로 추가되었다. Storage 에뮬레이터 연동 코드가 AppDelegate에서 제거되었으며, firebase.json의 경로가 firebase/ 하위로 일괄 정리되었다. 아래에서 실제 버그 가능성이 있는 항목을 지적한다.
| firestoreSettings.cacheSettings = MemoryCacheSettings() | ||
| Firestore.firestore().settings = firestoreSettings | ||
| Database.database().useEmulator(withHost: host, port: 9000) | ||
| Storage.storage().useEmulator(withHost: host, port: 9199) |
There was a problem hiding this comment.
[MEDIUM] Storage 에뮬레이터 연결 코드(Storage.storage().useEmulator(withHost: host, port: 9199))가 제거되었다. firebase.json에는 storage 에뮬레이터 설정이 그대로 남아 있는데, 앱에서 에뮬레이터로 연결하지 않으면 로컬 개발 환경에서 StorageProfileImageRepository가 실제 프로덕션 Storage에 요청을 보내게 된다. 의도적 제거인지, 또는 에뮬레이터 포트 설정이 다른 곳으로 이동되었는지 확인이 필요하다.
| } | ||
|
|
||
| let storage = self.storage | ||
| let task = Task<[String], Error> { |
There was a problem hiding this comment.
[HIGH] actor 내부에서 Task { } 를 직접 생성하면 해당 Task는 actor context 바깥의 unstructured task로 생성된다. 즉, namesFetchTask에 할당 직후 다른 caller가 fetchAvailableImageNames()를 동시에 호출하면, namesFetchTask가 아직 nil인 타이밍과 할당 완료 타이밍 사이에 여러 Task가 중복 생성될 수 있다. actor는 메서드 단위로 직렬화되므로 await 없이 연속 호출되는 경우 문제가 없지만, Task { } 내부 본문은 actor 격리 없이 실행되어 namesFetchTask, cachedNames 접근 시 actor 격리 보장이 실제로 유지되는지 재검토가 필요하다. 특히 task.value를 await하는 시점에 actor lock이 해제되므로, 그 사이에 다른 caller가 진입해 namesFetchTask가 nil인 상태를 볼 수 있다 — 이 경우 중복 네트워크 요청이 발생한다.
| self?.showLoadingIndicator() | ||
| } | ||
|
|
||
| Task { [weak self] in |
There was a problem hiding this comment.
[MEDIUM] 이미지 로딩 Task(라인 24~30)가 취소되지 않는다. setProfileImage가 재호출될 때 이전 Task는 계속 실행되다가 완료 후 self?.currentProfileIdentifier == identifier 비교로 이미지 적용은 막히지만, Task 자체는 계속 실행되어 불필요한 네트워크 요청과 데이터 수신이 발생한다. 셀 재사용이 빈번한 UICollectionView/UITableView 환경에서는 누적 Task로 인해 네트워크 과부하가 생길 수 있다. 이전 Task를 저장해 두고 cancel()을 호출하는 방식이 필요하다.
| isCircularImage: true, | ||
| accessoryView: profileEditIcon | ||
| ) | ||
| row.setProfileIcon(viewModel.profile.profileImageName) |
There was a problem hiding this comment.
[MEDIUM] profileRow의 lazy 초기화 블록에서 viewModel.profile.profileImageName을 참조하고 있다. 그런데 User.profileImage가 String으로 변경된 이후 profileImageName 프로퍼티를 제공하던 User+ProfileImage.swift가 이번 PR에서 삭제되었다. profileImageName이 여전히 다른 경로로 노출되고 있다면 문제없지만, 삭제된 extension이 유일한 소스였다면 컴파일 오류 또는 런타임에 잘못된 값이 사용된다. diff에서 profileImageName 참조가 라인 68과 207에 남아 있으므로, 해당 프로퍼티가 어디서 정의되는지 확인이 필요하다.
| addMarker(for: participant, color: colors[index]) | ||
| } | ||
|
|
||
| Task { |
There was a problem hiding this comment.
[MEDIUM] 마커 추가를 위한 Task { }가 구조화되지 않은 unstructured task로 생성되어 있다. AppointmentRouteMapView가 해제된 이후에도 Task가 완료될 때까지 실행이 계속되며, addMarker가 NMFMarker 등 지도 관련 객체를 조작하므로 해제된 뷰에서 지도 오브젝트에 접근할 가능성이 있다. [weak self] 캡처 또는 Task 취소 처리가 없으므로 메모리 및 크래시 위험이 있다.
There was a problem hiding this comment.
🤖 AI 코드 리뷰 - 버그 관점
이 PR은 프로필 이미지 저장소를 로컬 에셋에서 Firebase Storage 기반으로 전환한다. User.profileImage 타입을 URL에서 String(식별자)으로 변경하고, StorageProfileImageRepository와 ProfileImageLoader 싱글턴을 새로 추가하며, UIImageView에 비동기 로딩 extension을 붙인다. 에뮬레이터 환경에서 Storage 에뮬레이터 연결 설정이 제거되어 로컬 개발 시 실제 Storage에 접근하게 된다. StorageProfileImageRepository가 actor로 선언되어 있으나 내부 Task 생성 방식에 중복 호출 시 경쟁 조건이 남아 있으며, MyPageViewController에서 삭제된 profileImageName 프로퍼티를 계속 참조하는 코드가 존재한다.
| @@ -39,7 +39,6 @@ class AppDelegate: UIResponder, UIApplicationDelegate { | |||
| firestoreSettings.cacheSettings = MemoryCacheSettings() | |||
| Firestore.firestore().settings = firestoreSettings | |||
| Database.database().useEmulator(withHost: host, port: 9000) | |||
There was a problem hiding this comment.
[HIGH] Storage 에뮬레이터 연결 코드(Storage.storage().useEmulator(withHost: host, port: 9199))가 제거되었다. #if DEBUG 블록 내에 Firestore/Database 에뮬레이터 설정은 남아 있는데 Storage만 빠진 상태이므로, 개발/테스트 빌드에서 프로필 이미지 업로드·다운로드가 실제 Firebase Storage 버킷에 연결된다. 의도적인 변경인지 확인이 필요하다. 의도하지 않은 경우 버킷 접근 권한 오류 또는 실 데이터 오염이 발생할 수 있다.
| } | ||
|
|
||
| let storage = self.storage | ||
| let task = Task<[String], Error> { |
There was a problem hiding this comment.
[HIGH] fetchAvailableImageNames()가 actor 내에서 Task { ... }를 생성해 namesFetchTask에 저장하는 구조인데, Task 내부 코드는 actor 격리 밖에서 실행된다. 구체적으로 namesFetchTask = task(RIGHT:37) 할당 후 await task.value(RIGHT:40)를 기다리는 사이에 또 다른 호출자가 진입하면 RIGHT:26의 if let existing = namesFetchTask 분기를 타게 되어 올바르게 동작하지만, Task 클로저 내부에서 cachedNames = names(RIGHT:41)와 namesFetchTask = nil(RIGHT:42)을 수행하는 것은 actor 격리 없이 실행되므로 actor 격리된 저장 프로퍼티에 대한 접근이 컴파일 경고 또는 런타임 문제를 유발할 수 있다. Task 클로저는 actor 격리를 상속하지 않으므로, 격리된 프로퍼티 변경은 await MainActor.run 또는 actor 격리 메서드 내 직접 호출로 수행해야 한다. 현재 코드는 Swift actor 격리 규칙 위반으로 컴파일러가 오류를 발생시킬 가능성이 높다.
| isCircularImage: true, | ||
| accessoryView: profileEditIcon | ||
| ) | ||
| row.setProfileIcon(viewModel.profile.profileImageName) |
There was a problem hiding this comment.
[HIGH] viewModel.profile.profileImageName을 참조하고 있으나, 이 PR에서 User+ProfileImage.swift가 삭제되어 profileImageName 프로퍼티가 더 이상 존재하지 않는다. 같은 파일 RIGHT:207에서도 동일하게 profile.profileImageName을 사용한다. User.profileImage(String)를 직접 사용해야 하며, 현재 상태로는 빌드 오류가 발생한다.
|
|
||
| profileRow.value = profile.nickname | ||
| profileRow.setIcon(UIImage(named: profile.profileImageName) ?? UIImage(systemName: profile.profileImageName)) | ||
| profileRow.setProfileIcon(profile.profileImageName) |
There was a problem hiding this comment.
[HIGH] RIGHT:68과 동일하게 profile.profileImageName을 참조하고 있다. User+ProfileImage.swift 삭제로 이 프로퍼티가 존재하지 않으므로 빌드 오류가 발생한다.
| self?.showLoadingIndicator() | ||
| } | ||
|
|
||
| Task { [weak self] in |
There was a problem hiding this comment.
[MEDIUM] 이미지 로딩 Task(RIGHT:24~30)가 완료된 후 self?.image = loaded를 설정하는데, 이 코드가 메인 스레드에서 실행된다는 보장이 없다. ProfileImageLoader.load()가 내부적으로 메인 스레드 전환을 수행하지 않는 이상, UIImageView의 image 프로퍼티를 백그라운드 스레드에서 변경하게 되어 UI 업데이트 스레드 위반이 발생한다. await MainActor.run 또는 @MainActor 컨텍스트에서 이미지를 설정해야 한다.
| addMarker(for: participant, color: colors[index]) | ||
| } | ||
|
|
||
| Task { |
There was a problem hiding this comment.
[MEDIUM] Task { for (index, participant) in participants.enumerated() { await addMarker(...) } }가 메인 스레드 컨텍스트 외부에서 실행될 수 있다. addMarker는 NMFMarker 생성 및 지도 오버레이 추가를 수행할 것으로 예상되는데(RIGHT:112 이하), 이 작업이 메인 스레드에서 수행되어야 한다면 명시적으로 Task { @MainActor in ... } 또는 내부에서 await MainActor.run을 사용해야 한다.
snughnu
left a comment
snughnu
left a comment
There was a problem hiding this comment.
ai 리뷰에서 비슷한 생각이 드는 리뷰는 표시해뒀습니다!
🙊 수고하셨습니다!
| Task { | ||
| for (index, participant) in participants.enumerated() { | ||
| await addMarker(for: participant, color: colors[index]) | ||
| } |
There was a problem hiding this comment.
현재 for 루프 안에서, 매 참가자마다 await addMarker()를 순차적으로 호출하는데
그러면 참가자가 여러명일 때 마커가 하나씩 순서대로 나타나고, 네트워크 지연이 있으면 마지막 참가자 마커는 꽤 늦게 그려질거 같아요.
병렬 로드 후, 개별 또는 한번에 마커를 추가하는 등의 방법을 생각해볼 수도 있을 것 같습니다!
2 participants
JIRA
📝 작업 내용
📌 요약
🔍 상세
[1] 프로필 이미지 저장소 계층
ProfileImageRepository: 이미지 이름 목록 조회 + 이미지 데이터 다운로드 프로토콜 (Domain)StorageProfileImageRepository: Firebase Storageavatars/디렉토리 기반 구현체 (Data, actor)이름 목록 캐싱·Task 중복 방지·이미지 Data 캐싱을 actor 격리로 동시성 안전하게 처리
ProfileImageLoader: Repository에서 받은 Data를 UIImage로 변환하는 싱글턴 (Presentation)Firebase 의존 없음, 상태 없음
UIImageView+ProfileImage: 비동기 이미지 로딩 extensionobjc_setAssociatedObject로 셀 재사용 시 이미지 엇갈림 방지UIView+LoadingIndicator: 범용 로딩 인디케이터 extensionshowLoadingIndicator()/hideLoadingIndicator()로 UIView 어디서든 사용 가능[2] Domain/Data 타입 변경
User.profileImage:URL→StringFirestore 문서의 식별자 문자열(
"shark","basic"등)을 그대로 사용UserDTO,FirestoreUserRepository, Mock 4개:URL(string:)!force unwrap 전부 제거User+ProfileImage.swift삭제 — URL의.host에서 이름을 추출하던 computed property 불필요[3] Presentation 계층 전환
AppointmentRouteParticipant.profileImageURL: URL→profileImage: StringParticipant,SharedPlaceItem,ChatViewModel:profileImageName참조를profileImage로 통일UIImage(named:)→imageView.setProfileImage()전환대상:
ParticipantBox,OtherChatBubble,SharedPlaceRow,MyPageRow,MyPageViewController,ProfileImageCell,ParticipantRouteRowAppointmentRouteMapView.addMarker(): async로 변경,ProfileImageLoader로 마커 이미지 로드[4] 프로필 편집 화면
ProfileImageLoader.fetchAvailableImageNames()로 Storage에서 동적 조회questionmark.circleSF Symbol 표시: 로딩 중 (인디케이터 회전) → 성공 시 불러온 아바타 → 실패 시 심볼 표시[5] 기타
Assets.xcassets/Profile/디렉토리 삭제 (otter, shark, turtle)AppDelegate: Storage 에뮬레이터 설정 제거 — Storage만 프로덕션 사용 (Auth/Firestore는 에뮬레이터 유지)AppDIContainer+Register:StorageProfileImageRepositoryDI 등록💬 리뷰 노트
StorageProfileImageRepository를 actor로 만든 이유
fetchAvailableImageNames()에서cachedNames와namesFetchTask를await중단점 전후로 읽고 씁니다.plain class면 여러 Task가 동시에 접근할 때 data race가 발생합니다.
actor를 쓰면 중단점 전까지의 상태 변경이 원자적으로 처리되고,
await이후 재진입 시 이미 설정된namesFetchTask를 재사용해 중복 네트워크 호출을 방지합니다.Storage 에뮬레이터 제거
아바타 이미지 26개가 프로덕션 Storage에 업로드되어 있어서, 디버그에서도 Storage만 프로덕션을 바라보도록 변경했습니다.
에뮬레이터 Storage에 같은 이미지를 넣는 방법도 있었지만, 프로필 이미지는 읽기 전용이라 프로덕션 직접 사용이 더 단순합니다.
ProfileImageLoader의 역할 범위
Data→UIImage 변환과
failureImage반환만 담당하고, 캐싱·Task 중복 방지·Firebase 통신은 모두 Repository에 두었습니다.UIImage 캐시도 Repository의 Data 캐시로 대체했습니다 — 아바타가 1MB 이하 소형이라 매번 디코딩해도 무시할 수 있는 수준입니다.
기본 이미지 설정
사용자가 처음 회원가입 하면 기본 이미지로 설정될 이미지를 새로 만들었습니다.
로딩 인디케이터 150ms 딜레이
Task.sleep(for: .milliseconds(150))후에만 스피너를 표시하고, 그 전에 로드가 끝나면 Task를 취소해 스피너가 아예 나타나지 않도록 했습니다.150ms는 사람이 지연을 인지하기 시작하는 임계값에 맞춘 값입니다.
루트 디렉토리 정리
Firebase 파일이 루트에 복잡하게 많아 정리했습니다.
루트에 존재해야 하는 파일을 제외하고 하위 디렉토리로 정리했습니다.
추후 작업
📸 영상 / 이미지
Xcode 빌드로만 실기기 테스트가 가능해 느리게 동작하는 걸로 보입니다.
WORK52.avatar.MP4