Conversation
khyeji98
left a comment
khyeji98
left a comment
There was a problem hiding this comment.
현재 App에서 직접 KakaoSDK 의존성을 갖고 있는데, 이러한 의존관계는 추후 소셜 로그인을 추가하면서 로그인 피처가 디벨롭 되면 변경될 수도 있는걸까용?
각 객체가 어디에 존재하는지와 현재의 의존관계가 추후 변경되는건지, 지금의 구조 및 의존 관계가 의도된 것인지가 궁금했습니다!
| .SPM.kakaoMapsSDK, | ||
| .external(name: "KakaoSDKCommon"), | ||
| .external(name: "KakaoSDKAuth"), | ||
| .external(name: "KakaoSDKUser") |
There was a problem hiding this comment.
KakaoMapsSDK 처럼 SPM 접근 helper를 사용하면 통일성도 있고, 좀 더 안전할 것 같아용!
There was a problem hiding this comment.
Dependency로 뺴는게 나은가요?
저는 최대한 외부 의존성과 격리를 시키려고 했어요. 그리고 App에서 카카오 관련 초기화를 하니깐 App에다가 구현을 했습니다
|
|
||
| private extension LoginView { | ||
|
|
||
| var kakaoLoginButton: some View { |
There was a problem hiding this comment.
서브뷰의 경우 struct로 정의하는 것을 룰로 정했던 것 같은데, 룰이긴 하지만 개인 선호의 영역으로 가져가도 상관없는 것 같아서 이 부분에서의 여송님 의견이 궁금합니다!
There was a problem hiding this comment.
엇 수정 하도록 할게요! 그래도 일관성있게 가는게 좋은것 같아서요
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
|
엥 테스트했을 때 리뷰 잘 됐었는데, 확인 한번 해볼게용! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| public var baseURL: String { | ||
| // TODO: baseURL secret파일에 추가하고 가져오기 | ||
| let url = "https://port-0-bangawo-server-dev-mnaf2uuja2b612e3.sel3.cloudtype.app" |
There was a problem hiding this comment.
🔵 [P4] Readability
API baseURL이 코드에 하드코딩되어 있습니다. TODO 코멘트가 있지만, 빌드 스킴 또는 Tuist Config.xcconfig 등을 통해 환경 변수로 관리하여 개발/스테이징/운영 환경별로 유연하게 대응하고, 보안 및 유지보수성을 높이는 것이 좋습니다.
| // Copyright (c) 2025 DDD, Ltd., All rights reserved. | ||
| // | ||
|
|
||
| import Model |
There was a problem hiding this comment.
🟠 [P2] Major
Domain/DataInterface 모듈이 Data/Model 모듈을 의존하는 것은 Clean Architecture의 Presentation → Domain ← Data 흐름을 위반합니다. Domain 레이어는 Data 레이어의 구체 타입에 의존해서는 안 됩니다. AuthRepositoryProtocol에서 사용하는 LoginResponseDTO는 Domain/Entity 또는 Domain/DataInterface 자체 내에서 정의된 타입이어야 합니다.
| /// 인증 관련 데이터 소스 접근을 추상화하는 Repository 프로토콜 | ||
| public protocol AuthRepositoryProtocol: Sendable { | ||
| /// 소셜 로그인 토큰으로 서버 로그인을 요청합니다. | ||
| func login(provider: String, providerToken: String) async throws -> LoginResponseDTO |
There was a problem hiding this comment.
🟠 [P2] Major
AuthRepositoryProtocol의 login 메서드 반환 타입이 Data/Model의 LoginResponseDTO입니다. 이 프로토콜은 Domain/Entity의 AuthToken 타입을 반환하도록 변경하여 도메인 계층의 추상화를 유지해야 합니다.
| func login(provider: String, providerToken: String) async throws -> LoginResponseDTO | |
| func login(provider: String, providerToken: String) async throws -> AuthToken |
|
|
||
| public var body: some View { | ||
| VStack(spacing: 0) { | ||
| Spacer() |
There was a problem hiding this comment.
🔵 [P4] Readability
VStack 내에서 Spacer()를 사용하여 공간을 확장하는 대신, .frame(maxHeight: .infinity) 수식어를 사용하는 것이 좋습니다. 이는 Spacer가 View Hierarchy에 불필요한 레이아웃 요소를 추가하는 것을 방지하고, 뷰의 의도를 명확히 합니다.
| Spacer() | |
| Color.clear.frame(maxHeight: .infinity) |
| .pretendardCustomFont(textStyle: .heading0) | ||
| .foregroundStyle(.navy900) | ||
|
|
||
| Spacer() |
There was a problem hiding this comment.
🔵 [P4] Readability
VStack 내에서 Spacer()를 사용하여 공간을 확장하는 대신, .frame(maxHeight: .infinity) 수식어를 사용하는 것이 좋습니다. 이는 Spacer가 View Hierarchy에 불필요한 레이아웃 요소를 추가하는 것을 방지하고, 뷰의 의도를 명확히 합니다.
| Spacer() | |
| Color.clear.frame(maxHeight: .infinity) |
| .frame(maxWidth: .infinity) | ||
| .frame(height: 52) | ||
| .foregroundStyle(.gray900) | ||
| .background(Color(hex: "FEE500")) |
There was a problem hiding this comment.
🔵 [P4] Readability
하드코딩된 #FEE500 색상 대신 DesignSystem에 정의된 .kakaoYellow와 같은 이름 있는 색상 토큰을 사용해야 합니다. 이는 디자인 일관성과 유지보수성을 높입니다.
| .background(Color(hex: "FEE500")) | |
| .background(.designSystemKakaoYellow) |
| var body: some View { | ||
| Button(action: onTap) { | ||
| HStack(spacing: 8) { | ||
| Image(systemName: "g.circle.fill") |
There was a problem hiding this comment.
⚪ [P5] Nitpick
PR 설명에 언급된 대로, 애플 로그인 버튼 UI가 구글 로그인 아이콘(g.circle.fill)으로 잘못 표시되어 있습니다. 애플 로그인 아이콘(apple.logo 또는 apple.fill)으로 수정해야 합니다.
| .frame(maxWidth: .infinity) | ||
| .frame(height: 52) | ||
| .foregroundStyle(.gray100) | ||
| .background(Color(hex: "03C75A")) |
There was a problem hiding this comment.
🔵 [P4] Readability
하드코딩된 #03C75A 색상 대신 DesignSystem에 정의된 .naverGreen과 같은 이름 있는 색상 토큰을 사용해야 합니다. 이는 디자인 일관성과 유지보수성을 높입니다.
| .background(Color(hex: "03C75A")) | |
| .background(.designSystemNaverGreen) |
| .Presentation(implements: .Presentation), | ||
| .SPM.kakaoMapsSDK | ||
| .project(target: "CoreDependencies", path: .relativeToRoot("Projects/Core/Dependencies")), | ||
| .project(target: "DataUseCase", path: .relativeToRoot("Projects/Data/UseCase")), |
There was a problem hiding this comment.
🟠 [P2] Major
App 모듈에서 DataUseCase, Data.Repository, Data.Service 등 구체적인 데이터 계층 구현체를 직접 의존하는 것은 모듈 아키텍처 원칙 (Presentation → Domain ← Data)에 위배될 수 있습니다. App 모듈은 의존성 주입을 위한 Composition Root 역할을 하지만, Presentation 계층이 이러한 구체적인 Data 계층 구현체에 직접 의존하지 않도록 Domain 계층의 추상화를 통해 간접적으로 연결해야 합니다.
현재 AuthAssembly가 이 데이터 계층 컴포넌트들을 조합하고 있으므로, App 모듈은 AuthAssembly가 포함된 모듈 (예: App 자체)에만 의존하고, AuthAssembly가 필요한 Data 계층 모듈들을 내부적으로 가져오는 것이 더 깔끔합니다. AuthAssembly가 App 모듈 내에 있으므로, 이 Data 계층 의존성들이 AuthAssembly.swift가 아닌 App/Project.swift에 직접 명시된 점은 아쉬움이 있습니다.
| .project(target: "DataUseCase", path: .relativeToRoot("Projects/Data/UseCase")), | |
| .project(target: "CoreDependencies", path: .relativeToRoot("Projects/Core/Dependencies")), | |
| .Presentation(implements: .Presentation) | |
| // AuthAssembly가 내부적으로 필요한 Data 계층 의존성을 가져오도록 구조 개선 고려 |
| .SPM.kakaoMapsSDK | ||
| .project(target: "CoreDependencies", path: .relativeToRoot("Projects/Core/Dependencies")), | ||
| .project(target: "DataUseCase", path: .relativeToRoot("Projects/Data/UseCase")), | ||
| .Data(implements: .Repository), |
There was a problem hiding this comment.
🟠 [P2] Major
App 모듈에서 DataUseCase, Data.Repository, Data.Service 등 구체적인 데이터 계층 구현체를 직접 의존하는 것은 모듈 아키텍처 원칙 (Presentation → Domain ← Data)에 위배될 수 있습니다. App 모듈은 의존성 주입을 위한 Composition Root 역할을 하지만, Presentation 계층이 이러한 구체적인 Data 계층 구현체에 직접 의존하지 않도록 Domain 계층의 추상화를 통해 간접적으로 연결해야 합니다.
현재 AuthAssembly가 이 데이터 계층 컴포넌트들을 조합하고 있으므로, App 모듈은 AuthAssembly가 포함된 모듈 (예: App 자체)에만 의존하고, AuthAssembly가 필요한 Data 계층 모듈들을 내부적으로 가져오는 것이 더 깔끔합니다. AuthAssembly가 App 모듈 내에 있으므로, 이 Data 계층 의존성들이 AuthAssembly.swift가 아닌 App/Project.swift에 직접 명시된 점은 아쉬움이 있습니다.
| .Data(implements: .Repository), | |
| .project(target: "CoreDependencies", path: .relativeToRoot("Projects/Core/Dependencies")), | |
| .Presentation(implements: .Presentation) | |
| // AuthAssembly가 내부적으로 필요한 Data 계층 의존성을 가져오도록 구조 개선 고려 |
There was a problem hiding this comment.
app모듈은 조립을 위한 루트 역할을 하므로 Data 계층 구현체를 조립하기 위해 의존하는 것은 의도했습니다.
Presentation계층은 Data계층을 직접 의존하지 않도록 분리했는데, 어떻게 생각하시나요?
There was a problem hiding this comment.
App 모듈에서 의존성 조립을 위해 Data 의존성을 갖고, 대신 Presentation 모듈에서 Data 의존성을 갖지 않도록 설계하셨다는 말씀이신거죵?
개인 톡으로 소통했던 것처럼 해당 구조를 가져가는 것에 동의합니다!
| .project(target: "CoreDependencies", path: .relativeToRoot("Projects/Core/Dependencies")), | ||
| .project(target: "DataUseCase", path: .relativeToRoot("Projects/Data/UseCase")), | ||
| .Data(implements: .Repository), | ||
| .Data(implements: .Service), |
There was a problem hiding this comment.
🟠 [P2] Major
App 모듈에서 DataUseCase, Data.Repository, Data.Service 등 구체적인 데이터 계층 구현체를 직접 의존하는 것은 모듈 아키텍처 원칙 (Presentation → Domain ← Data)에 위배될 수 있습니다. App 모듈은 의존성 주입을 위한 Composition Root 역할을 하지만, Presentation 계층이 이러한 구체적인 Data 계층 구현체에 직접 의존하지 않도록 Domain 계층의 추상화를 통해 간접적으로 연결해야 합니다.
현재 AuthAssembly가 이 데이터 계층 컴포넌트들을 조합하고 있으므로, App 모듈은 AuthAssembly가 포함된 모듈 (예: App 자체)에만 의존하고, AuthAssembly가 필요한 Data 계층 모듈들을 내부적으로 가져오는 것이 더 깔끔합니다. AuthAssembly가 App 모듈 내에 있으므로, 이 Data 계층 의존성들이 AuthAssembly.swift가 아닌 App/Project.swift에 직접 명시된 점은 아쉬움이 있습니다.
| .Data(implements: .Service), | |
| .project(target: "CoreDependencies", path: .relativeToRoot("Projects/Core/Dependencies")), | |
| .Presentation(implements: .Presentation) | |
| // AuthAssembly가 내부적으로 필요한 Data 계층 의존성을 가져오도록 구조 개선 고려 |
|
|
||
| public var baseURL: String { | ||
| // TODO: baseURL secret파일에 추가하고 가져오기 | ||
| let url = "https://port-0-bangawo-server-dev-mnaf2uuja2b612e3.sel3.cloudtype.app" |
There was a problem hiding this comment.
🟡 [P3] Minor
baseURL이 코드에 하드코딩되어 있습니다. TODO 주석이 있지만, 환경에 따라 변경될 수 있는 민감한 정보는 Tuist의 Config.xcconfig 파일 등을 통해 관리하고 빌드 시점에 주입하는 것이 바람직합니다.
| let url = "https://port-0-bangawo-server-dev-mnaf2uuja2b612e3.sel3.cloudtype.app" | |
| // TODO: baseURL을 Config.xcconfig 파일에서 가져오도록 수정 | |
| // guard let url = Bundle.main.infoDictionary?["BASE_URL"] as? String else { | |
| // fatalError("BASE_URL not set in Info.plist") | |
| // } | |
| // return url |
| // | ||
|
|
||
| import Foundation | ||
| import Entity |
There was a problem hiding this comment.
🟠 [P2] Major
Data.Model 모듈 (AuthDTO.swift 파일)이 Domain.Entity 모듈을 import하는 것은 모듈 의존성 방향 (Presentation → Domain ← Data)을 위반합니다. DTO (Data Transfer Object)는 순수하게 데이터 구조를 정의하며, Domain 계층의 Entity를 알아서는 안 됩니다. DTO를 Entity로 변환하는 매핑 로직은 Data.Repository 또는 Data.Service 계층에서 이루어져야 합니다.
| import Entity | |
| // import Entity |
There was a problem hiding this comment.
DTO가 entity에 의존하는거 자체는 괜찮다고 생각하는데, 변환 책임까지 가져서 이런 리뷰가 달린게 아닌가 싶은데 따로 Mapper를 두는게 나을까요? 아니면 약간의 트레이드 오프를 가지고 좀 더 단순하고 직관적인 이 방식으로 가는게 나을까요?
There was a problem hiding this comment.
저도 작성해주신 형태처럼 작성한 경험이 많아용
다만 모델이라는 모듈을 분리하다보니 트레이드오프를 가져가지 않는 방법도 고려되네용
Mapper를 둔다면, Mapper 객체를 정의하시나요? 아니면 extension으로 확장하시나용?
There was a problem hiding this comment.
저는 extension으로 많이 하는데, 현재는 필드와 1:1 맵핑이 되니깐 굳이 Mapper를 만드는게 이점이 많은거 같지 않아서요. 그대로 가겠습니다.
| /// registrationCompleted가 true인 회원만 메인화면으로 아니면 회원가입 화면 | ||
| } | ||
|
|
||
| public extension LoginResponseDTO { // 엔티티 변환 함수 추가 |
There was a problem hiding this comment.
🟠 [P2] Major
LoginResponseDTO에 toEntity() 확장 함수가 포함되어 있어 Data.Model 모듈이 Domain.Entity에 의존하게 됩니다. 이 매핑 로직은 Data.Repository 또는 Data.Service 계층에서 수행되어야 합니다. DTO는 데이터 전송만을 목적으로 해야 하며, Domain 계층의 모델 변환 로직을 포함하지 않는 것이 좋습니다.
| public extension LoginResponseDTO { // 엔티티 변환 함수 추가 | |
| // 이 확장 함수는 AuthRepositoryImpl 또는 KakaoLoginService와 같은 Data 계층의 구현체로 이동해야 합니다. | |
| // public extension LoginResponseDTO { | |
| // func toEntity() -> AuthToken { | |
| // AuthToken( | |
| // accessToken: accessToken, | |
| // refreshToken: refreshToken, | |
| // isNewMember: isNewMember, | |
| // registrationCompleted: registrationCompleted | |
| // ) | |
| // } | |
| // } |
| providerToken: providerToken | ||
| ) | ||
|
|
||
| KeyChainManager.addItem(key: KeyChainKey.accessToken, value: response.accessToken) |
There was a problem hiding this comment.
🟠 [P2] Major
UseCase 구현체 (SignInWithSocialUseCaseImpl)에서 KeyChainManager를 직접 호출하여 토큰을 저장하는 것은 레이어링 원칙 위반입니다. 키체인 접근과 같은 데이터 영속성 처리는 Data 계층의 Repository가 담당해야 합니다. AuthRepositoryProtocol에 saveAuthToken(_ token: AuthToken)과 같은 메서드를 추가하고, AuthRepositoryImpl에서 KeyChainManager를 사용하여 구현하도록 변경하는 것이 좋습니다.
| KeyChainManager.addItem(key: KeyChainKey.accessToken, value: response.accessToken) | |
| // AuthRepositoryProtocol에 토큰 저장 메서드를 추가하고 해당 메서드를 호출하도록 변경 | |
| // self.repository.saveAuthToken(response) |
| ) | ||
|
|
||
| KeyChainManager.addItem(key: KeyChainKey.accessToken, value: response.accessToken) | ||
| KeyChainManager.addItem(key: KeyChainKey.refreshToken, value: response.refreshToken) |
There was a problem hiding this comment.
🟠 [P2] Major
UseCase 구현체 (SignInWithSocialUseCaseImpl)에서 KeyChainManager를 직접 호출하여 토큰을 저장하는 것은 레이어링 원칙 위반입니다. 키체인 접근과 같은 데이터 영속성 처리는 Data 계층의 Repository가 담당해야 합니다. AuthRepositoryProtocol에 saveAuthToken(_ token: AuthToken)과 같은 메서드를 추가하고, AuthRepositoryImpl에서 KeyChainManager를 사용하여 구현하도록 변경하는 것이 좋습니다.
| KeyChainManager.addItem(key: KeyChainKey.refreshToken, value: response.refreshToken) | |
| // AuthRepositoryProtocol에 토큰 저장 메서드를 추가하고 해당 메서드를 호출하도록 변경 | |
| // self.repository.saveAuthToken(response) |
| var body: some View { | ||
| Button(action: onTap) { | ||
| HStack(spacing: 8) { | ||
| Image(systemName: "g.circle.fill") |
There was a problem hiding this comment.
🔵 [P4] Readability
애플 로그인 버튼에 Google을 상징하는 g.circle.fill 시스템 이미지가 사용되었습니다. PR 설명에서 '애플로그인 버튼 UI를 구글로그인으로 잘못표기 -> 추후 수정 예정'이라고 언급되었지만, 실제 코드는 여전히 잘못된 이미지를 사용하고 있습니다. 올바른 애플 로고 이미지(예: apple.logo)로 수정해야 합니다.
| Image(systemName: "g.circle.fill") | |
| Image(systemName: "apple.logo") |
| public struct AuthToken: Equatable, Sendable { | ||
| public let accessToken: String | ||
| public let refreshToken: String | ||
| public let isNewMember: Bool | ||
| public let registrationCompleted: Bool |
There was a problem hiding this comment.
두 토큰값을 별도 프로퍼티로 하지 않고 Token 모델을 분리하면 추후 토큰 갱신시 모델 재사용이 가능할 것 같고, 현재 모델을 로그인 응답 결과에 대한 네이밍으로 좀 더 명확하게 나타낼 수 있을 것 같다는 생각을 했어용
이 부분에 대해서 여송님 의견 궁금합니다!
| import Repository | ||
| import Service | ||
|
|
||
| enum AuthAssembly { |
There was a problem hiding this comment.
의존성 생성 역할을 좀 더 명확하게 드러내기 위해 아래와 같은 네이밍으로 패턴에 대한 이해도를 함께 가져가면 어떨까용?
| enum AuthAssembly { | |
| enum AuthFactory { |
| } withDependencies: { | ||
| $0.socialAuthClient = AuthAssembly.makeSocialAuthClient() | ||
| } |
There was a problem hiding this comment.
의존성 주입 코드가 명확하게 분리되니 가독성도 좋고, 패턴화가 되어 좋은 아이디어 같아용👍
| var body: some View { | ||
| Button(action: onTap) { | ||
| HStack(spacing: 8) { | ||
| Image(systemName: "g.circle.fill") |
There was a problem hiding this comment.
🟡 [P3] Minor
Apple 로그인 버튼에 Google 아이콘(g.circle.fill)이 사용되었습니다. PR 설명에서 언급된 대로, Apple 아이콘(apple.logo 등)으로 수정하는 것이 좋습니다.
| import Utill | ||
|
|
||
| /// 인증 관련 Repository 구현체 | ||
| public struct AuthRepositoryImpl: AuthRepositoryProtocol { |
There was a problem hiding this comment.
🔵 [P4] Readability
Repository 구현체는 일반적으로 struct보다는 final class로 선언하여 참조 의미론을 명확히 하고, 상속을 방지하여 최적화를 돕는 것이 좋습니다. 이 변경은 성능상 큰 차이를 만들지 않더라도 Swift 코드 품질 가이드라인에 더 부합합니다.
| public struct AuthRepositoryImpl: AuthRepositoryProtocol { | |
| public final class AuthRepositoryImpl: AuthRepositoryProtocol { |
| static let composableArchitecture = TargetDependency.external(name: "ComposableArchitecture", condition: .none) | ||
| static let sharing = TargetDependency.external(name: "Sharing", condition: .none) | ||
| static let kakaoMapsSDK = TargetDependency.external(name: "KakaoMapsSDK-SPM", condition: .none) | ||
| static let kakaoSDKCommon = TargetDependency.external(name: "KakaoSDKCommon", condition: .none) |
There was a problem hiding this comment.
⚪ [P5] Nitpick
condition: .none은 불필요하게 명시적인 경우가 많습니다. 기본값이 .none이므로 생략 가능합니다.
| static let kakaoSDKCommon = TargetDependency.external(name: "KakaoSDKCommon", condition: .none) | |
| static let kakaoSDKCommon = TargetDependency.external(name: "KakaoSDKCommon") |
| static let sharing = TargetDependency.external(name: "Sharing", condition: .none) | ||
| static let kakaoMapsSDK = TargetDependency.external(name: "KakaoMapsSDK-SPM", condition: .none) | ||
| static let kakaoSDKCommon = TargetDependency.external(name: "KakaoSDKCommon", condition: .none) | ||
| static let kakaoSDKAuth = TargetDependency.external(name: "KakaoSDKAuth", condition: .none) |
There was a problem hiding this comment.
⚪ [P5] Nitpick
condition: .none은 불필요하게 명시적인 경우가 많습니다. 기본값이 .none이므로 생략 가능합니다.
| static let kakaoSDKAuth = TargetDependency.external(name: "KakaoSDKAuth", condition: .none) | |
| static let kakaoSDKAuth = TargetDependency.external(name: "KakaoSDKAuth") |
| static let kakaoMapsSDK = TargetDependency.external(name: "KakaoMapsSDK-SPM", condition: .none) | ||
| static let kakaoSDKCommon = TargetDependency.external(name: "KakaoSDKCommon", condition: .none) | ||
| static let kakaoSDKAuth = TargetDependency.external(name: "KakaoSDKAuth", condition: .none) | ||
| static let kakaoSDKUser = TargetDependency.external(name: "KakaoSDKUser", condition: .none) |
There was a problem hiding this comment.
⚪ [P5] Nitpick
condition: .none은 불필요하게 명시적인 경우가 많습니다. 기본값이 .none이므로 생략 가능합니다.
| static let kakaoSDKUser = TargetDependency.external(name: "KakaoSDKUser", condition: .none) | |
| static let kakaoSDKUser = TargetDependency.external(name: "KakaoSDKUser") |
| } | ||
|
|
||
| // 매개변수 없는 URL 타입 (예: 카카오) | ||
| // 매개변수 없는 URL 타입 |
There was a problem hiding this comment.
🔵 [P4] Readability
해당 주석은 기존 코드에서 삭제된 주석으로, 변경 사항 없이 복사된 것으로 보입니다. 불필요한 주석은 제거하는 것이 좋습니다.
| "CFBundleURLSchemes": .array([ | ||
| .string("${REVERSED_CLIENT_ID}") | ||
| // .string("com.googleusercontent.apps.882277748169-glpolfiecue4lqqps6hmgj9t8lm1g5qp") | ||
| .string("$(REVERSED_CLIENT_ID)"), |
There was a problem hiding this comment.
⚪ [P5] Nitpick
REVERSED_CLIENT_ID는 카카오와 관련이 없는 ID입니다. 카카오 URL Scheme은 kakao$(KAKAO_APP_KEY)만 필요합니다. 불필요한 스킴은 제거하는 것이 좋습니다.
| .string("$(REVERSED_CLIENT_ID)"), | |
| .string("kakao$(KAKAO_APP_KEY)") |
| /// registrationCompleted가 true인 회원만 메인화면으로 아니면 회원가입 화면 | ||
| } | ||
|
|
||
| public extension LoginResponseDTO { // 엔티티 변환 함수 추가 |
There was a problem hiding this comment.
🟠 [P2] Major
Data/Model 모듈에 위치한 LoginResponseDTO의 toEntity() 확장 함수는 Domain/Entity 타입인 LoginResult와 AuthTokens를 직접 사용하고 있습니다. 이는 Data 모듈이 Domain 모듈에 의존하게 되는 의존성 역전 문제를 야기합니다. DTO-Entity 변환은 Repository 계층의 구현체 (AuthRepositoryImpl)에서 이루어져야 합니다.
다음과 같이 AuthRepositoryImpl에서 변환을 처리하도록 수정하는 것을 권장합니다.
| public extension LoginResponseDTO { // 엔티티 변환 함수 추가 | |
| // LoginResponseDTO에서는 toEntity()를 삭제합니다. | |
| // Projects/Data/Repository/Sources/AuthRepositoryImpl.swift | |
| public final class AuthRepositoryImpl: AuthRepositoryProtocol { | |
| // ... | |
| public func login(provider: String, providerToken: String) async throws -> LoginResult { | |
| let requestDTO = LoginRequestDTO(provider: provider, providerToken: providerToken) | |
| let responseDTO: LoginResponseDTO = try await NetworkManager.shared.request( | |
| AuthEndPoint.login(requestDTO) | |
| ) | |
| return LoginResult( | |
| tokens: AuthTokens( | |
| accessToken: responseDTO.accessToken, | |
| refreshToken: responseDTO.refreshToken | |
| ), | |
| isNewMember: responseDTO.isNewMember, | |
| registrationCompleted: responseDTO.registrationCompleted | |
| ) | |
| } | |
| // ... | |
| } |
|
|
||
| public var body: some View { | ||
| VStack(spacing: 0) { | ||
| Spacer() |
There was a problem hiding this comment.
🔵 [P4] Readability
단순한 공간 확장을 위해 Spacer()를 사용하는 대신, frame(maxHeight: .infinity) 또는 Spacer(minLength: 0)와 함께 .weight(1)을 사용하여 SwiftUI 레이아웃을 더 명시적으로 제어할 수 있습니다.
| .pretendardCustomFont(textStyle: .heading0) | ||
| .foregroundStyle(.navy900) | ||
|
|
||
| Spacer() |
There was a problem hiding this comment.
🔵 [P4] Readability
단순한 공간 확장을 위해 Spacer()를 사용하는 대신, frame(maxHeight: .infinity) 또는 Spacer(minLength: 0)와 함께 .weight(1)을 사용하여 SwiftUI 레이아웃을 더 명시적으로 제어할 수 있습니다.
| var body: some View { | ||
| Button(action: onTap) { | ||
| HStack(spacing: 8) { | ||
| Image(systemName: "apple.logo") |
There was a problem hiding this comment.
🟡 [P3] Minor
PR 설명에 '애플로그인 버튼 UI를 구글로그인으로 잘못표기 -> 추후 수정 예정'이라고 되어있습니다. 현재 Image(systemName: "apple.logo")는 정확한 애플 로고이므로, 이 부분은 수정할 필요 없이 올바르게 구현된 것으로 보입니다. 혹시 이 Image에 대한 수정 예정 사항이라면, 구글 로고 이미지로 변경하기 위한 명확한 시스템 이름이 필요합니다.
| var body: some View { | ||
| Button(action: onTap) { | ||
| HStack(spacing: 8) { | ||
| Text("N") |
There was a problem hiding this comment.
🟡 [P3] Minor
네이버 로고는 보통 심볼 또는 전용 이미지를 사용합니다. 단순 Text("N")으로 표현하는 것은 UI/UX 가이드라인에 맞지 않을 수 있습니다. 네이버에서 제공하는 SDK에 포함된 이미지 리소스나 공식적인 디자인 시스템에 맞는 아이콘을 사용하는 것을 고려해 주세요.
2 participants
Summary
Notes
Screenshot