Conversation
📝 WalkthroughWalkthrough새로운 Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 상세 리뷰 분석긍정적 측면 ✨1. 잘 구조화된 에러 처리 아키텍처
2. BaseViewModel의 일관된 리팩토링
3. 모듈 구조의 명확한 계층화
개선 고려사항 🤔1. ResponseException 파싱 안전성 검토 try {
val errorBody = response.body?.string()
val parsed = errorJson.decodeFromString<ErrorResponse>(errorBody.orEmpty())
// 사용
} catch (e: Exception) {
// 폴백 처리
}질문: ResponseException에서 response body 파싱 시 실패한다면 어떻게 처리되나요? 만약 에러 응답이 JSON이 아닌 경우나 변신(serialization) 실패 시, 원본 body 정보가 손실될 수 있습니다. 다음과 같은 개선은 어떨까요? AppError.Http(
status = status,
code = parsed?.code,
message = parsed?.message,
rawBody = errorBody // 파싱 실패해도 원본 유지
)2. errorJson 설정의 문서화
3. BaseViewModel의 cancellation 처리 catch (e: CancellationException) {
throw e // 재발생
}질문: 현재 구현에서 CancellationException을 재발생시키는데, 이것이 상위 coroutine scope로 전파되나요? 아니면 finally 블록이 실행되고 Job은 정상 종료되나요? 의도한 동작이 명확하도록 주석을 추가하면 나중에 혼동을 방지할 수 있습니다. 4. Unknown 에러의 필수 Throwable data class Unknown(val cause: Throwable) : AppError다른 AppError 변형은 optional cause를 가지는데 Unknown만 필수입니다. 이것이 의도된 설계인지, 아니면 일관성을 위해 optional로 변경해야 하는지 확인이 필요합니다. 5. ErrorResponse의 확장성
확인 사항 ✓
전반적으로 새로운 에러 처리 아키텍처가 타입 안전하고 체계적으로 설계되었다는 점이 매우 좋습니다. 위의 질문들은 디테일을 다듬기 위한 제안이므로, 개발 맥락에 따라 조정하시면 될 것 같습니다. 🚀 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/ui/src/main/java/com/twix/ui/base/BaseViewModel.kt (1)
80-105:⚠️ Potential issue | 🟠 MajorlaunchResult에서 예외 처리 불일치 - 일관된 에러 핸들링이 필요합니다
현재
launchResult메서드는CancellationException만 처리하고 다른 예외는 전파되고 있어요. 이는 같은 클래스의init블록(36-45줄)에서 적용한 예외 처리 패턴과 다릅니다.문제점:
block()실행 중 예외 발생 시handleError와onError콜백이 호출되지 않음- 예외가
viewModelScope로 전파되어 코루틴 스코프가 취소될 수 있음- 호출 측에서
onError콜백을 통한 에러 처리를 기대하지만 예기치 않게 동작하지 않을 수 있음이미
AppError.Unknown(cause: Throwable)과handleError(error: AppError)오버로드가 존재하므로,init블록의 패턴(try-catch-all + handleError 호출)을launchResult에도 동일하게 적용하면 됩니다:🛡️ 제안 변경
} catch (e: CancellationException) { // 코루틴 취소는 에러로 취급하지 않기 throw e + } catch (t: Throwable) { + val appError = AppError.Unknown(t) + handleError(appError) + onError?.invoke(appError) } finally { onFinally?.invoke() }
🤖 Fix all issues with AI agents
In `@core/network/src/main/java/com/twix/network/execute/ApiCall.kt`:
- Around line 8-45: The catch block in safeApiCall is importing and catching
kotlinx.io.IOException which differs from the java.io.IOException that
AppError.Network expects (see AppError.kt); update ApiCall.kt to import
java.io.IOException instead of kotlinx.io.IOException so the catch (e:
IOException) in safeApiCall correctly captures Ktor/Java IO errors and maps them
to AppError.Network.
- Around line 27-32: The two runCatching blocks that compute raw (calling
e.response.bodyAsText()) and parsed (calling
errorJson.decodeFromString<ErrorResponse>(it)) swallow CancellationException and
prevent coroutine cancellation from propagating; update those runCatching usages
so they rethrow CancellationException (or explicitly check and throw if caught)
instead of converting it to null, ensuring CancellationException is not
swallowed when reading bodyAsText() or decoding JSON in the raw/parsed logic.
🧹 Nitpick comments (1)
core/network/src/main/java/com/twix/network/model/error/ErrorResponse.kt (1)
5-10: 잘 구현된 에러 응답 모델입니다!모든 필드를 nullable로 설정하여 다양한 서버 에러 응답 형식을 유연하게 처리할 수 있도록 설계되었습니다.
한 가지 고려해볼 점이 있습니다: 만약 서버 API 스펙에서 특정 필드(예:
message)가 항상 반환된다고 보장된다면, 해당 필드를 non-nullable로 변경하면 사용하는 측에서 불필요한 null 체크를 줄일 수 있습니다. 현재 서버 스펙이 다양하거나 불확실한 경우라면 현재 방식이 적절합니다.서버 에러 응답 스펙이 어떻게 정의되어 있나요?
| import kotlinx.io.IOException | ||
| import kotlinx.serialization.SerializationException | ||
| import kotlinx.serialization.json.Json | ||
| import java.net.SocketTimeoutException | ||
| import kotlin.coroutines.cancellation.CancellationException | ||
|
|
||
| val errorJson = | ||
| Json { | ||
| ignoreUnknownKeys = true | ||
| isLenient = true | ||
| } | ||
|
|
||
| suspend inline fun <T> safeApiCall(crossinline call: suspend () -> T): AppResult<T> = | ||
| try { | ||
| AppResult.Success(call()) | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: ResponseException) { | ||
| val status = e.response.status.value | ||
| val raw = runCatching { e.response.bodyAsText() }.getOrNull() | ||
|
|
||
| val parsed = | ||
| runCatching { | ||
| raw?.let { errorJson.decodeFromString<ErrorResponse>(it) } | ||
| }.getOrNull() | ||
|
|
||
| AppResult.Error( | ||
| AppError.Http( | ||
| status = status, | ||
| code = parsed?.code, | ||
| message = parsed?.message, | ||
| rawBody = raw, | ||
| ), | ||
| ) | ||
| } catch (e: SocketTimeoutException) { | ||
| AppResult.Error(AppError.Timeout(e)) | ||
| } catch (e: IOException) { | ||
| AppResult.Error(AppError.Network(e)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find AppError definition
fd -t f -e kt -e java | xargs rg "class AppError|sealed.*AppError|data class Network" -B 2 -A 5 | head -100Repository: YAPP-Github/Twix-Android
Length of output: 1353
🏁 Script executed:
# Search for AppError definition more specifically
find . -type f \( -name "*.kt" -o -name "*.java" \) -exec rg "AppError" {} + -l | head -20Repository: YAPP-Github/Twix-Android
Length of output: 96
🏁 Script executed:
# Look for the AppError class definition
rg "class AppError|sealed.*AppError" -t kt -t java -A 10Repository: YAPP-Github/Twix-Android
Length of output: 94
🏁 Script executed:
cat core/result/src/main/java/com/twix/result/AppError.ktRepository: YAPP-Github/Twix-Android
Length of output: 827
🏁 Script executed:
# Check if there's any usage of kotlinx.io.IOException in the codebase
rg "kotlinx\.io\.IOException|java\.io\.IOException" --color=neverRepository: YAPP-Github/Twix-Android
Length of output: 241
🏁 Script executed:
# Check the imports and structure of ApiCall.kt
cat core/network/src/main/java/com/twix/network/execute/ApiCall.ktRepository: YAPP-Github/Twix-Android
Length of output: 1666
kotlinx.io.IOException을 java.io.IOException으로 변경 필요
현재 코드의 문제점:
AppError.Network는java.io.IOException을 기대합니다 (AppError.kt에서java.io.IOExceptionimport)- 그런데
ApiCall.kt에서는kotlinx.io.IOException을 catch하고 있습니다 - 이 둘은 서로 다른 타입이므로, Ktor에서 발생하는
java.io.IOException은 catch 블록에 걸리지 않습니다 - 결과적으로 네트워크 오류가
catch (e: Throwable)블록으로 빠져AppError.Unknown으로 처리됩니다
import를 다음과 같이 수정하면 네트워크 오류가 올바르게 AppError.Network로 처리됩니다:
-import kotlinx.io.IOException
+import java.io.IOException📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import kotlinx.io.IOException | |
| import kotlinx.serialization.SerializationException | |
| import kotlinx.serialization.json.Json | |
| import java.net.SocketTimeoutException | |
| import kotlin.coroutines.cancellation.CancellationException | |
| val errorJson = | |
| Json { | |
| ignoreUnknownKeys = true | |
| isLenient = true | |
| } | |
| suspend inline fun <T> safeApiCall(crossinline call: suspend () -> T): AppResult<T> = | |
| try { | |
| AppResult.Success(call()) | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: ResponseException) { | |
| val status = e.response.status.value | |
| val raw = runCatching { e.response.bodyAsText() }.getOrNull() | |
| val parsed = | |
| runCatching { | |
| raw?.let { errorJson.decodeFromString<ErrorResponse>(it) } | |
| }.getOrNull() | |
| AppResult.Error( | |
| AppError.Http( | |
| status = status, | |
| code = parsed?.code, | |
| message = parsed?.message, | |
| rawBody = raw, | |
| ), | |
| ) | |
| } catch (e: SocketTimeoutException) { | |
| AppResult.Error(AppError.Timeout(e)) | |
| } catch (e: IOException) { | |
| AppResult.Error(AppError.Network(e)) | |
| import java.io.IOException | |
| import kotlinx.serialization.SerializationException | |
| import kotlinx.serialization.json.Json | |
| import java.net.SocketTimeoutException | |
| import kotlin.coroutines.cancellation.CancellationException | |
| val errorJson = | |
| Json { | |
| ignoreUnknownKeys = true | |
| isLenient = true | |
| } | |
| suspend inline fun <T> safeApiCall(crossinline call: suspend () -> T): AppResult<T> = | |
| try { | |
| AppResult.Success(call()) | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: ResponseException) { | |
| val status = e.response.status.value | |
| val raw = runCatching { e.response.bodyAsText() }.getOrNull() | |
| val parsed = | |
| runCatching { | |
| raw?.let { errorJson.decodeFromString<ErrorResponse>(it) } | |
| }.getOrNull() | |
| AppResult.Error( | |
| AppError.Http( | |
| status = status, | |
| code = parsed?.code, | |
| message = parsed?.message, | |
| rawBody = raw, | |
| ), | |
| ) | |
| } catch (e: SocketTimeoutException) { | |
| AppResult.Error(AppError.Timeout(e)) | |
| } catch (e: IOException) { | |
| AppResult.Error(AppError.Network(e)) |
🤖 Prompt for AI Agents
In `@core/network/src/main/java/com/twix/network/execute/ApiCall.kt` around lines
8 - 45, The catch block in safeApiCall is importing and catching
kotlinx.io.IOException which differs from the java.io.IOException that
AppError.Network expects (see AppError.kt); update ApiCall.kt to import
java.io.IOException instead of kotlinx.io.IOException so the catch (e:
IOException) in safeApiCall correctly captures Ktor/Java IO errors and maps them
to AppError.Network.
| val raw = runCatching { e.response.bodyAsText() }.getOrNull() | ||
|
|
||
| val parsed = | ||
| runCatching { | ||
| raw?.let { errorJson.decodeFromString<ErrorResponse>(it) } | ||
| }.getOrNull() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ApiCall.kt" | head -20Repository: YAPP-Github/Twix-Android
Length of output: 132
🏁 Script executed:
git ls-files | grep -E "network|ApiCall" | head -20Repository: YAPP-Github/Twix-Android
Length of output: 1001
🏁 Script executed:
cat -n core/network/src/main/java/com/twix/network/execute/ApiCall.kt | head -50Repository: YAPP-Github/Twix-Android
Length of output: 2016
🌐 Web query:
Kotlin runCatching CancellationException coroutine cancellation propagation
💡 Result:
runCatching { ... } catches all Throwable from its block—this includes CancellationException. In coroutines, CancellationException is the mechanism used to stop work and propagate cancellation through suspension points; swallowing it breaks cancellation propagation (the coroutine may keep running until the next cancellable suspension/check, and parent/child cancellation behavior becomes surprising). (kotlinlang.org)
What to do instead
If you use runCatching inside coroutines, rethrow cancellation:
val result = runCatching {
doWork()
}.onFailure { e ->
if (e is CancellationException) throw e
}or equivalently:
val result = try {
Result.success(doWork())
} catch (e: CancellationException) {
throw e
} catch (t: Throwable) {
Result.failure(t)
}Kotlin’s coroutine docs explicitly warn that catching CancellationException can break cancellation propagation and that you should rethrow it. (kotlinlang.org)
Why it matters (rule of thumb)
CancellationExceptionis treated specially by coroutine exception handling (it’s generally ignored by handlers) because it represents normal cancellation, not an error. (kotlinlang.org)- Therefore, any “catch-all” (
catch (Throwable)/runCatching) in coroutine code should always letCancellationExceptionescape. (medium.com)
Citations:
- 1: https://kotlinlang.org/docs/cancellation-and-timeouts.html?utm_source=openai
- 2: https://kotlinlang.org/docs/cancellation-and-timeouts.html?utm_source=openai
- 3: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines/-coroutine-exception-handler/index.html?utm_source=openai
- 4: https://medium.com/mobilepeople/how-to-break-a-coroutine-cancellation-12e968c31642?utm_source=openai
🏁 Script executed:
rg "runCatching" --type kotlin -A 3 | head -60Repository: YAPP-Github/Twix-Android
Length of output: 1164
🏁 Script executed:
cat -n core/network/src/main/java/com/twix/network/execute/ApiCall.kt | sed -n '20,42p'Repository: YAPP-Github/Twix-Android
Length of output: 980
runCatching이 코루틴 취소를 막을 수 있어요
runCatching은 CancellationException을 포함한 모든 Throwable을 잡기 때문에, bodyAsText()나 JSON 파싱 중에 취소가 발생해도 예외가 getOrNull()로 인해 null로 변환되어 취소 신호가 전파되지 않습니다. 이는 코루틴 취소 의도를 무효화하고 상위 코루틴까지 취소가 전파되지 않는 문제를 일으킵니다.
Kotlin 공식 문서에서도 코루틴 내에서 runCatching을 사용할 때는 반드시 CancellationException을 재throw할 것을 권장하고 있습니다. 두 runCatching 호출 모두에 다음과 같이 취소 예외 처리를 추가해 주세요.
🔧 제안 변경
- val raw = runCatching { e.response.bodyAsText() }.getOrNull()
+ val raw =
+ runCatching { e.response.bodyAsText() }
+ .onFailure { if (it is CancellationException) throw it }
+ .getOrNull()
...
- val parsed =
- runCatching {
- raw?.let { errorJson.decodeFromString<ErrorResponse>(it) }
- }.getOrNull()
+ val parsed =
+ runCatching {
+ raw?.let { errorJson.decodeFromString<ErrorResponse>(it) }
+ }
+ .onFailure { if (it is CancellationException) throw it }
+ .getOrNull()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val raw = runCatching { e.response.bodyAsText() }.getOrNull() | |
| val parsed = | |
| runCatching { | |
| raw?.let { errorJson.decodeFromString<ErrorResponse>(it) } | |
| }.getOrNull() | |
| val raw = | |
| runCatching { e.response.bodyAsText() } | |
| .onFailure { if (it is CancellationException) throw it } | |
| .getOrNull() | |
| val parsed = | |
| runCatching { | |
| raw?.let { errorJson.decodeFromString<ErrorResponse>(it) } | |
| } | |
| .onFailure { if (it is CancellationException) throw it } | |
| .getOrNull() |
🤖 Prompt for AI Agents
In `@core/network/src/main/java/com/twix/network/execute/ApiCall.kt` around lines
27 - 32, The two runCatching blocks that compute raw (calling
e.response.bodyAsText()) and parsed (calling
errorJson.decodeFromString<ErrorResponse>(it)) swallow CancellationException and
prevent coroutine cancellation from propagating; update those runCatching usages
so they rethrow CancellationException (or explicitly check and throw if caught)
instead of converting it to null, ensuring CancellationException is not
swallowed when reading bodyAsText() or decoding JSON in the raw/parsed logic.
2 participants
이슈 번호
작업내용
리뷰어에게 추가로 요구하는 사항 (선택)
뷰모델에서 Repository 메서드 호출할 때 BaseViewModel launchResult 호출하시면 됩니다!
그리고 RepositoryImpl에서 로직 구현할 때도 safeApiCall 기반으로 통신해주세요