Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
전반적으로 TCA, 모듈 아키텍처, SwiftUI 컨벤션을 잘 준수하고 있습니다. 특히 소셜 로그인 시 추천 닉네임 연동, 약관 API 연동, 닉네임 검증 로직 추가 및 requestVoid와 같은 네트워크 계층 확장은 PR의 내용을 잘 반영하고 있습니다.
다만, 일부 긴 코드 라인과 함수 시그니처, 그리고 TermClause의 displayTitle 계산 로직에서 중요한 버그가 발견되어 수정이 필요합니다. 또한, 로깅 관련 잠재적 민감 정보 노출과 테스트 코드의 프로덕션 코드 잔류 문제도 개선하면 좋겠습니다.
| get: { store.imagePicker != nil }, | ||
| set: { if !$0 { store.send(.imagePicker(.dismiss)) } } | ||
| set: { | ||
| if !$0, store.imagePicker != nil { |
There was a problem hiding this comment.
⚪ [P5] Nitpick
if !$0, store.imagePicker != nil 조건에서 store.imagePicker != nil은 불필요합니다. isPresented 바인딩이 false로 설정되는 시점에는 이미 시트가 닫히고 있거나 닫혔음을 의미하므로, if !$0만으로도 충분합니다.
| public let isRequired: Bool | ||
|
|
||
| public init(id: String, title: String, body: String) { | ||
| public init(id: Int, title: String, body: String, isRequired: Bool = true) { |
There was a problem hiding this comment.
🔵 [P4] Readability
isRequired 파라미터에 true라는 기본값을 할당했습니다. 이 값은 항상 서버 응답에서 제공되는 것으로 예상되므로, Presentation 계층의 TermClause 이니셜라이저에는 기본값을 제거하는 것이 더 명확하고 데이터 소스의 신뢰성을 높일 수 있습니다. 만약 스토리보드나 프리뷰를 위한 값이라면, 별도의 static var preview 등을 만들어서 사용하는 것을 권장합니다.
| // TermPDFViewer.swift | ||
| // Presentation | ||
| // | ||
| // TermClause 본문 텍스트를 PDFKit으로 표시하는 임시 placeholder 뷰어 |
There was a problem hiding this comment.
⚪ [P5] Nitpick
텍스트를 PDF로 렌더링하는 현재 방식은 간단한 텍스트에는 적합하지만, 복잡한 레이아웃이나 대량의 텍스트에는 비효율적일 수 있습니다. 주석에 언급된 대로, 추후 실제 PDF URL을 기반으로 PDFDocument(url:)을 사용하거나, UITextView 또는 WKWebView를 통해 더 유연하게 콘텐츠를 표시하는 방향으로 확장하는 것이 좋습니다.
|
회원가입 플로우 API 연동 및 관련 기능 개선 작업에 대한 코드 리뷰입니다. 전반적으로 TCA 컨벤션, 모듈 아키텍처, Swift 코드 품질 등 제시된 모든 가이드라인을 잘 준수하고 있습니다. 특히, 비동기 작업 처리, 상태 관리, 의존성 주입( |
khyeji98
left a comment
khyeji98
left a comment
There was a problem hiding this comment.
우선순위가 높은 코멘트들은 아니지만 확인 한번 부탁드려용!
그리고 TokenRefreshService의 경우 별도 모듈로 분리하는 방식을 제안드리긴 했는데, 한번 고민해보시고 올려주셔도 좋고 아니면 해당 pr은 일단 머지해주시고 추가 작업으로 가져가주셔도 좋습니당👍
| // 직접 키체인에 접근하는 것이 아닌 repository를 통해 접근하도록 | ||
| repository.saveAuthTokens(loginResult.tokens) | ||
| repository.saveRegistrationCompleted(loginResult.registrationCompleted) | ||
|
|
There was a problem hiding this comment.
🔵 [P4] Readability
UserDefaults.standard.set(true, forKey: UserDefaultsKey.isLogin)는 로그인 성공 시 isLogin 상태를 true로 설정하는 로직입니다. 이 로직은 RootFeature의 onAppear에서 hasToken과 registrationCompleted를 함께 확인하는 로직과 밀접하게 관련되어 있어, 로그인 성공 후 AuthRepositoryProtocol을 통해 registrationCompleted와 같은 영구 저장소에 저장하는 것이 일관성을 유지하고 책임 분리에 더 적합합니다. isLogin 또한 Repository에 위임하여 처리하는 것이 좋습니다.
| repository.saveRegistrationCompleted(loginResult.registrationCompleted) | |
| repository.saveIsLogin(true) // 새로운 메서드 추가 제안 |
| do { | ||
| let decodedData = try JSONDecoder().decode(T.self, from: data) | ||
| Log.debug("✅ [Decode Success] \(T.self)") | ||
| Log.debug("✅ [Decode Success] \(decodedData)") |
There was a problem hiding this comment.
🟡 [P3] Minor
디코드 성공 시 디버그 로그에 decodedData를 직접 출력하면 민감한 정보가 포함될 수 있습니다. T.self와 같이 타입 정보를 로그하거나, Decodable 프로토콜에 debugDescription을 추가하여 제어하는 것이 좋습니다.
| Log.debug("✅ [Decode Success] \(decodedData)") | |
| Log.debug("✅ [Decode Success] \(T.self)") |
|
|
||
| case .registerMemberResponse(.success): | ||
| state.isRegistering = false | ||
| UserDefaults.standard.set(true, forKey: UserDefaultsKey.registrationCompleted) // 회원가입 완료시에 true로 갱신 |
There was a problem hiding this comment.
🔵 [P4] Readability
AuthFlowFeature에서 UserDefaults.standard.set을 직접 호출하는 대신, registrationCompleted 저장 로직을 AuthRepositoryProtocol에 위임하는 것이 좋습니다. 이는 데이터 계층이 영구 저장소 관련 책임을 전담하도록 하여 아키텍처의 일관성을 유지하고 테스트 용이성을 높일 수 있습니다.
| UserDefaults.standard.set(true, forKey: UserDefaultsKey.registrationCompleted) // 회원가입 완료시에 true로 갱신 | |
| // AuthRepository 또는 AuthUseCase에 위임하여 처리 | |
| // repository.saveRegistrationCompleted(result.registrationCompleted) |
| case .registerMemberResponse(.success): | ||
| state.isRegistering = false | ||
| UserDefaults.standard.set(true, forKey: UserDefaultsKey.registrationCompleted) // 회원가입 완료시에 true로 갱신 | ||
| UserDefaults.standard.set(true, forKey: UserDefaultsKey.isLogin) |
There was a problem hiding this comment.
🔵 [P4] Readability
AuthFlowFeature에서 UserDefaults.standard.set(true, forKey: UserDefaultsKey.isLogin)을 직접 호출하는 대신, isLogin 상태 저장 로직을 AuthRepositoryProtocol에 위임하는 것이 좋습니다. 이는 SignInWithSocialUseCaseImpl에서 이미 isLogin을 설정하고 있으므로, 모든 로그인/회원가입 관련 영구 저장 로직을 데이터 계층에서 일관되게 관리할 수 있습니다.
| UserDefaults.standard.set(true, forKey: UserDefaultsKey.isLogin) | |
| // AuthRepository 또는 AuthUseCase에 위임하여 처리 | |
| // repository.saveIsLogin(true) // 이미 로그인 시 설정되므로 불필요할 수 있음 |
2 participants
작업 내용
회원가입 플로우 API 연동
닉네임 입력 검증 개선
약관 화면 개선
네트워크/데이터 계층 추가
requestVoid추가앱 설정 및 DI 조립
SignupTermsFactory,NicknameFactory추가BangawoApp.prepareDependencies에 신규 client 주입