Conversation
🤖 Generated with [Firebender](https://firebender.com) Co-Authored-By: Firebender <help@firebender.com>
🤖 Generated with [Firebender](https://firebender.com) Co-Authored-By: Firebender <help@firebender.com>
🤖 Generated with [Firebender](https://firebender.com) Co-Authored-By: Firebender <help@firebender.com>
🤖 Generated with [Firebender](https://firebender.com) Co-Authored-By: Firebender <help@firebender.com>
📝 WalkthroughWalkthrough이 변경사항은 기존 단일 데이터 모듈에서 의존성을 분리하여 Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 상세 리뷰✅ 긍정적인 측면아키텍처 계층화가 잘 구성되었습니다.
모듈 구조 분리가 명확합니다.
🤔 개선을 위해 고려해볼 점들1. DefaultRemoteAnswerDataSource의 JSON 처리 방식현재 코드: val json = Json { ignoreUnknownKeys = true }논의가 필요한 부분:
2. 여러 데이터소스에서 JSON 설정 반복DefaultRemoteAnswerDataSource, DefaultRemoteExamDataSource, DefaultRemoteQuestionDataSource에서 모두 개선 방향: // core/network의 HttpClient.kt처럼 별도 싱글톤으로 관리
object JsonSerializer {
val instance = Json { ignoreUnknownKeys = true }
}이렇게 하면 설정이 일관되고 유지보수가 용이합니다. 3. DefaultLocalAuthDataSource의 역할 명확화현재 상황:
질문:
4. PostgrestFilter와 PostgrestOrder의 검증현재 코드: data class PostgrestFilter(val column: String, val value: Any)
data class PostgrestOrder(val column: String, val descending: Boolean = false)개선 제안:
5. core/network/build.gradle.kts의 BuildKonfig 로딩val properties = Properties()
val localPropertiesFile = rootProject.file("local.properties")
if (localPropertiesFile.exists()) {
properties.load(localPropertiesFile.inputStream())
}고려사항:
6. HttpClient 싱글톤의 지연 초기화val client by lazy {
createSupabaseClient(...)
}논의가 필요한 부분:
7. 기존 data/src/commonMain/kotlin/com/peto/droidmorning/data/di/NetworkModule.kt 및 DataStoreModule.kt 삭제확인 필요:
8. SupabasePostgrestClient의 쿼리 구성코드 리뷰 포인트: // select 메서드에서 filter 적용
query.select().eq("column", value)이 구문이 여러 필터가 있을 때 AND 조건으로 결합되는지 명확하신가요? 문서나 테스트로 보장되는지 확인이 필요합니다. 📋 추가 확인 항목
전반적으로 이 PR은 아키텍처를 개선하는 긍정적인 방향의 변경입니다. 위의 지적사항들이 해결되면 더욱 견고한 코드베이스가 될 것 같습니다. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/datastore/src/iosMain/kotlin/com/peto/droidmorning/core/datastore/DataStoreFactory.ios.kt (1)
13-19: Null 안전성 문제가 있습니다.
documentDirectory가NSURL?타입이므로 null일 수 있습니다. 현재 코드에서documentDirectory?.path가 null일 경우, 문자열 연결 시 Kotlin이 이를"null"문자열로 변환하여"null/TOKEN_DATASTORE_FILENAME"형태의 유효하지 않은 경로가 생성됩니다.이로 인해 iOS에서 앱 시작 시 DataStore 초기화가 실패할 수 있습니다.
🐛 Null 체크를 추가한 수정 제안
actual fun createTokenDataStore(): DataStore<Preferences> = PreferenceDataStoreFactory.createWithPath( produceFile = { - val documentDirectory: NSURL? = documentDirectory - (documentDirectory?.path + "/" + TOKEN_DATASTORE_FILENAME).toPath() + val directory = requireNotNull(documentDirectory?.path) { + "iOS document directory could not be found" + } + "$directory/$TOKEN_DATASTORE_FILENAME".toPath() }, )또는 더 안전하게 처리하려면:
actual fun createTokenDataStore(): DataStore<Preferences> = PreferenceDataStoreFactory.createWithPath( produceFile = { val path = documentDirectory?.path ?: throw IllegalStateException("iOS document directory is unavailable") "$path/$TOKEN_DATASTORE_FILENAME".toPath() }, )이렇게 하면 null인 경우 명확한 오류 메시지와 함께 실패하여 디버깅이 용이해집니다.
🤖 Fix all issues with AI agents
In `@core/network/build.gradle.kts`:
- Around line 32-53: props.getProperty(...) 값을 그대로 buildConfigField에 넣어 null을
전달하는 문제를 방지하려면 누락된 프로퍼티를 검사하는 헬퍼를 추가하고 기존 호출을 해당 헬퍼로 교체하세요: 생성한
requireProperty(key: String) 또는 getRequiredProp(key) 함수는 props.getProperty(key)를
읽고 null 또는 빈값이면 명확한 예외(설정해야 하는 키 이름과 안내 메시지 포함)를 던지도록 구현하고,
buildConfigField(Type.STRING, "GOOGLE_CLIENT_ID",
requireProperty("GOOGLE_CLIENT_ID")) 등으로 변경해 빌드 타임에 실패하도록 만드세요; props,
getProperty, buildConfigField와 같은 심볼을 찾아 수정하세요.
In
`@core/network/src/commonMain/kotlin/com/peto/droidmorning/core/network/HttpClient.kt`:
- Around line 9-16: client를 lazy로 생성할 때 BuildKonfig.SUPABASE_URL 또는
BuildKonfig.SUPABASE_KEY가 누락되거나 빈 문자열일 경우 런타임 예외로 이어질 수 있으므로, lazy 블록 내에서
require(...)를 사용해 BuildKonfig.SUPABASE_URL과 BuildKonfig.SUPABASE_KEY가 공백이 아님을 즉시
검증하도록 수정하세요; 구체적으로 client 변수를 초기화하는 createSupabaseClient 호출 바로 전에
require(BuildKonfig.SUPABASE_URL.isNotBlank())와
require(BuildKonfig.SUPABASE_KEY.isNotBlank()) 형태의 검증을 추가하고, 각각 명확한 오류 메시지를 제공하여
문제 발생 시 즉시 원인을 알 수 있게 하세요.
In
`@core/network/src/commonMain/kotlin/com/peto/droidmorning/core/network/SupabasePostgrestClient.kt`:
- Around line 55-83: The update and delete implementations in
SupabasePostgrestClient currently allow empty filters and thus risk modifying
the whole table; add a guard at the start of both update(...) and delete(...)
that enforces filters is not empty (e.g., require(filters.isNotEmpty()) or throw
IllegalArgumentException with a clear message referencing PostgrestFilter), or
add an explicit allowAll flag to the method signature and only permit empty
filters when that flag is true; apply this check before calling
postgrest.from(...).update(...) and postgrest.from(...).delete(...).
🧹 Nitpick comments (8)
core/datastore/src/androidMain/kotlin/com/peto/droidmorning/core/datastore/DataStoreFactory.android.kt (1)
16-19:dataStore프로퍼티를lazy초기화로 변경하면 의도가 더 명확해집니다.현재
get()getter를 사용하면 매 호출 시context.dataStoredelegate를 조회합니다.preferencesDataStoredelegate가 내부적으로 캐싱을 처리하므로 기능상 문제는 없지만,lazy를 사용하면 "한 번만 초기화한다"는 의도가 더 명확해지고 불필요한 delegate 조회 오버헤드도 제거됩니다.♻️ lazy 초기화 적용 제안
private object DataStoreProvider : KoinComponent { private val context: Context by inject() - val dataStore: DataStore<Preferences> - get() = context.dataStore + val dataStore: DataStore<Preferences> by lazy { context.dataStore } }core/datastore/src/iosMain/kotlin/com/peto/droidmorning/core/datastore/DataStoreFactory.ios.kt (1)
21-29: 에러 처리 개선을 고려해 보세요 (선택 사항).현재
error파라미터가null로 전달되어 디렉토리 조회 실패 시 원인을 파악하기 어렵습니다. iOS에서NSDocumentDirectory는 일반적으로 항상 사용 가능하지만, 디버깅 편의를 위해 에러 정보를 캡처하는 것도 좋은 방법입니다.다만, 이는 기존 코드 패턴을 유지하는 것이고 실제 문제가 발생할 가능성이 낮으므로, 추후 개선 사항으로 남겨두셔도 괜찮습니다.
build-logic/convention/src/main/kotlin/com/peto/droidmorning/extentions/ProjectExtensions.kt (1)
42-44: Koin Compose 의존성 범위와 버전 정합성 확인 필요왜 문제가 될 수 있나:
commonMain에koin-compose,koin-compose-viewmodel까지 항상 추가하면 Compose를 쓰지 않는 KMP 모듈/타깃에서 불필요한 의존성 증가나 호환성 문제가 생길 수 있습니다.
어떻게 개선할 수 있나: Compose 플러그인이 적용된 모듈에서만 추가하도록 가드하거나,koinDependencies()와 별도의koinComposeDependencies()로 분리하는 방식을 고려해 주세요. 또한 버전 카탈로그에서 Koin 버전이 4.1.1로 통일되어 있는지 확인 부탁드립니다. Based on learnings, Koin 4.1.1 규칙을 확인해주세요.♻️ 추천 개선안(Compose 플러그인 적용 시에만 추가)
internal fun Project.koinDependencies() { + val hasCompose = plugins.hasPlugin("org.jetbrains.compose") extensions.configure<KotlinMultiplatformExtension> { sourceSets.apply { commonMain { dependencies { implementation(libs.library("koin-core")) - implementation(libs.library("koin-compose")) - implementation(libs.library("koin-compose-viewmodel")) + if (hasCompose) { + implementation(libs.library("koin-compose")) + implementation(libs.library("koin-compose-viewmodel")) + } } } commonTest { dependencies { implementation(libs.library("koin-test")) } } } } }build-logic/convention/src/main/kotlin/com/peto/droidmorning/KotlinMultiplatformConventionPlugin.kt (1)
12-15: AndroidLibraryConventionPlugin 무조건 적용은 모듈 범위 확인 필요왜 문제가 될 수 있나: 이 컨벤션 플러그인이 non-Android KMP 모듈에도 적용된다면, Android 플러그인 적용으로 빌드 실패나 불필요한 AGP 의존성이 생길 수 있습니다.
어떻게 개선할 수 있나: Android 타깃이 있는 모듈에만 적용되도록 별도 컨벤션 플러그인으로 분리하거나, Gradle 속성으로 opt-in 하도록 가드하는 방식을 검토해 주세요. 모든 적용 대상 모듈이 Android 타깃을 포함하나요?settings.gradle.kts (1)
40-42: projectDir 매핑이 명시적으로 잘 설정되었습니다 (선택적 개선 가능)모듈 구조가 명확하게 설정되었습니다. 다만, Gradle은 기본적으로 모듈 이름에서 디렉토리 경로를 추론하므로, 이
projectDir매핑은 기술적으로 중복될 수 있습니다.예를 들어:
:core:network→ 자동으로core/network경로 추론:core:datastore→ 자동으로core/datastore경로 추론명시적 매핑은 가독성과 명확성을 높이는 장점이 있으므로, 팀 컨벤션에 따라 유지하거나 제거할 수 있습니다. 현재 상태로도 문제없이 동작합니다.
♻️ (선택) 중복 매핑 제거
include(":core") include(":core:network") include(":core:datastore") - -project(":core").projectDir = file("core") -project(":core:network").projectDir = file("core/network") -project(":core:datastore").projectDir = file("core/datastore")data/src/commonMain/kotlin/com/peto/droidmorning/data/datasource/answer/remote/DefaultRemoteAnswerDataSource.kt (1)
23-55: 역직렬화 예외의 도메인 변환 정책을 검토해 주세요.
decodeFromString실패 시SerializationException이 그대로 전파될 수 있습니다. 데이터 계층에서 도메인 에러로 매핑하거나Result로 감싸는 정책이 있다면 여기서 처리하는 편이 안전합니다. 필요 시 공통 헬퍼로 묶는 방향도 고려해 볼까요?코딩 가이드 기준입니다.
data/src/commonMain/kotlin/com/peto/droidmorning/data/datasource/exam/remote/DefaultRemoteExamDataSource.kt (1)
24-37: 응답 처리 패턴을 다른 RPC 호출과 일치시키는 것을 추천합니다.현재
submitExam은 RPC 응답을Long.serializer()로 직접 디코딩하고 있는데, 같은 파일의fetchExamDetail이나 다른 데이터소스의 모든 RPC 호출들(fetchAnswerHistory,fetchQuestionsByCategory등)은 응답 DTO를 사용하고 있습니다. 이런 패턴 불일치는 타입 안전성 측면에서 위험할 수 있습니다.백엔드의
create_exam_with_items함수가{ "id": 123 }같은 객체 형태로 응답한다면, 현재 코드는 런타임에 직렬화 실패로 이어질 수 있습니다. 코드라빗의 데이터 모듈 리뷰 가이드에서도 강조하듯이, 응답 DTO를 통해 명확한 책임 분리와 일관성 있는 에러 처리가 가능합니다.개선 예시
+@kotlinx.serialization.Serializable +data class CreateExamResponse(val id: Long) // ... - ).let { data -> - json.decodeFromString(Long.serializer(), data) - } + ).let { data -> + json.decodeFromString(CreateExamResponse.serializer(), data).id + }core/datastore/src/commonMain/kotlin/com/peto/droidmorning/core/datastore/DefaultTokenDataStore.kt (1)
12-28: DataStore 읽기 예외가 그대로 전파될 수 있습니다.
dataStore.data.first()에서 IOException이 발생하면 그대로 전파되어 로그인/자동로그인 플로우가 실패할 수 있습니다. 의도적으로 예외를 올리는 설계가 아니라면, 빈 Preferences로 폴백하는 처리를 고려해 주세요.♻️ 예외 폴백 처리 제안
import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.emptyPreferences import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.stringPreferencesKey +import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.first +import java.io.IOException @@ - private suspend fun preferences(): Preferences = dataStore.data.first() + private suspend fun preferences(): Preferences = + dataStore.data + .catch { exception -> + if (exception is IOException) emit(emptyPreferences()) else throw exception + } + .first()
core/network/src/commonMain/kotlin/com/peto/droidmorning/core/network/HttpClient.kt
Show resolved
Hide resolved
| override suspend fun update( | ||
| table: String, | ||
| body: JsonElement, | ||
| filters: List<PostgrestFilter>, | ||
| ) { | ||
| postgrest | ||
| .from(table) | ||
| .update(body) { | ||
| filter { | ||
| filters.forEach { filter -> | ||
| eq(filter.column, filter.value) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override suspend fun delete( | ||
| table: String, | ||
| filters: List<PostgrestFilter>, | ||
| ) { | ||
| postgrest | ||
| .from(table) | ||
| .delete { | ||
| filter { | ||
| filters.forEach { filter -> | ||
| eq(filter.column, filter.value) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
update/delete에서 빈 필터를 방지해야 합니다.
지금은 filters가 비어도 실행 가능해 전체 테이블 수정/삭제 위험이 있습니다. 실수 방지를 위해 최소 1개 필터를 강제하거나 별도의 “전체 대상” API로 분리하는 편이 안전합니다.
🛡️ 제안 수정
override suspend fun update(
table: String,
body: JsonElement,
filters: List<PostgrestFilter>,
) {
+ require(filters.isNotEmpty()) { "update requires at least one filter" }
postgrest
.from(table)
.update(body) {
filter {
filters.forEach { filter ->
eq(filter.column, filter.value)
}
}
}
}
override suspend fun delete(
table: String,
filters: List<PostgrestFilter>,
) {
+ require(filters.isNotEmpty()) { "delete requires at least one filter" }
postgrest
.from(table)
.delete {
filter {
filters.forEach { filter ->
eq(filter.column, filter.value)
}
}
}
}🤖 Prompt for AI Agents
In
`@core/network/src/commonMain/kotlin/com/peto/droidmorning/core/network/SupabasePostgrestClient.kt`
around lines 55 - 83, The update and delete implementations in
SupabasePostgrestClient currently allow empty filters and thus risk modifying
the whole table; add a guard at the start of both update(...) and delete(...)
that enforces filters is not empty (e.g., require(filters.isNotEmpty()) or throw
IllegalArgumentException with a clear message referencing PostgrestFilter), or
add an explicit allowAll flag to the method signature and only permit empty
filters when that flag is true; apply this check before calling
postgrest.from(...).update(...) and postgrest.from(...).delete(...).
1 participant
이슈 번호
#40
작업내용
core:network에 Supabase 클라이언트 래퍼(AuthClient,PostgrestClient)와 구현(SupabaseAuthClient,SupabasePostgrestClient)을 분리core:datastore에 토큰 저장소(TokenDataStore)와 플랫폼별 DataStore 팩토리 분리networkModule,dataStoreModule) 구성settings.gradle.kts에 core 모듈 등록KoinInitializer에 core 모듈 등록composeApp에core:network,core:datastore의존성 추가