Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| // Scheme url from backend is not matching with deeplink url in navigation | ||
| // So, let's navigate to correct deeplink from here | ||
| LaunchedEffect(navController) { |
There was a problem hiding this comment.
[Minor] LaunchedEffect 키로 navController 사용은 의미론적으로 부적절합니다.
LaunchedEffect(navController)는 "navController가 변경될 때 다시 실행"이라는 의미이지만, 실제 의도는 "컴포지션 진입 시 단 한 번 실행"입니다. rememberNavController()는 리컴포지션에서 동일한 인스턴스를 반환하므로 현재는 기능적으로 동일하게 동작하지만, 코드 의도가 불명확합니다.
LaunchedEffect(Unit)을 사용하면 의도가 더 명확해집니다:
| LaunchedEffect(navController) { | |
| LaunchedEffect(Unit) { |
추가로, targetId는 Activity intent에서 읽는 값이므로 변경되지 않습니다. Unit 키를 사용하는 것이 "딱 한 번 실행" 패턴에 부합합니다.
| ) { | ||
| navController.popBackStack() | ||
| } | ||
| LaunchedEffect(Unit) { |
There was a problem hiding this comment.
[Info] ✅ LaunchedEffect(Unit) 올바르게 사용하고 있습니다.
remember { } 안에서 Intent를 생성했던 기존 코드와 달리, Intent 생성 및 launcher.launch() 호출을 LaunchedEffect(Unit) 내부로 이동한 것은 올바른 패턴입니다. Unit 키를 사용해 리컴포지션과 무관하게 정확히 한 번만 실행되도록 보장하고 있으며, it.toRoute<CallvanNavType.CallvanChat>().postId 캡처도 NavBackStackEntry가 안정적이므로 안전합니다.
| val targetChatId = intent.getIntExtra(EXTRA_CHAT_ROOM_ID, -1) | ||
| val targetClubId = intent.getIntExtra(EXTRA_CLUB_ID, -1) | ||
| val targetEventId = intent.getIntExtra(EXTRA_EVENT_ID, -1) | ||
| val targetPostId = intent.getIntExtra(EXTRA_POST_ID, -1) |
There was a problem hiding this comment.
[Minor] EXTRA_POST_ID 미존재 시 -1이 GroupChatViewModel로 전파될 수 있습니다.
getIntExtra(EXTRA_POST_ID, -1)는 해당 Extra가 없을 때 -1을 반환합니다. 이 -1이 navigateTo에 Pair(EXTRA_POST_ID, -1)로 전달되면, GroupChatActivity가 실행될 경우 ViewModel에서 아래 코드가 실행됩니다:
val postId: Int? = savedStateHandle[EXTRA_POST_ID] // -1 (null이 아님)
checkNotNull(postId) // 통과됨
getCallvanPostDetail(-1) // 잘못된 API 호출SchemeActivity는 .filter { it.second != null }로 null을 걸러내지만, SplashActivity에서는 이러한 필터링이 없어 -1이 그대로 전달됩니다.
타입 callvan-chat일 때만 GroupChatActivity가 실행되고, 해당 Deep Link URL에는 항상 postId가 포함되어야 하므로 실제 발생 가능성은 낮습니다. 그러나 방어적으로 처리하려면:
val targetPostId = if (intent.hasExtra(EXTRA_POST_ID)) intent.getIntExtra(EXTRA_POST_ID, -1) else -1또는 기존 패턴과의 일관성을 위해 GroupChatViewModel에서 -1 검증을 추가하는 방법도 있습니다.
| } | ||
|
|
||
| private fun getPostIdFromUrl(url: String): Int? { | ||
| return url.toUri().getQueryParameter("postId")?.toIntOrNull() |
There was a problem hiding this comment.
[Trivial] 쿼리 파라미터 문자열 "postId"를 상수로 추출하는 것을 고려해볼 수 있습니다.
다른 메서드들도 동일한 패턴(하드코딩된 문자열)을 사용하고 있으므로 이는 기존 코드와 일관성이 있습니다. 다만 백엔드 API 파라미터 이름 변경 시 한 곳에서만 수정할 수 있도록 상수화하는 것을 추후 개선사항으로 고려해볼 수 있습니다.
현재 패턴과의 일관성 유지를 위해 이번 PR에서 수정하지 않아도 무방합니다.
| @@ -35,7 +35,6 @@ interface Navigator { | |||
| ): Intent | |||
|
|
|||
| fun navigateToGroupChat( | |||
There was a problem hiding this comment.
[Minor] Navigator 인터페이스에서 postId 제거로 인해 암묵적 계약이 형성됩니다.
기존에는 navigateToGroupChat(context, postId)처럼 인터페이스만 봐도 어떤 데이터가 필요한지 명확했습니다. 변경 후에는 호출자가 반드시 putExtra(EXTRA_POST_ID, ...)를 별도로 호출해야 한다는 사실을 알아야 하며, 이를 누락하면 GroupChatViewModel.checkNotNull(postId)에서 크래시가 발생합니다.
이 설계가 buildIntent 유틸리티와의 일관성을 위한 의도적인 선택임은 이해하나, 추후 다른 진입점에서 GroupChatActivity를 실행할 때 EXTRA_POST_ID를 빠뜨리는 실수가 생길 수 있습니다.
현재 호출부가 Navigation.kt 하나뿐이라 큰 문제는 아니지만, 장기적으로는 파라미터를 인터페이스에 유지하거나 KDoc 주석으로 요구사항을 명시하는 것을 권장합니다.
There was a problem hiding this comment.
parameter 에 postId 가 있는 편이 좋을 거 같아요
| CLUB("club", ClubActivity::class.java), | ||
|
|
||
| LOST_AND_FOUND("articles/lost-item", LostAndFoundActivity::class.java); | ||
| LOST_AND_FOUND("articles/lost-item", LostAndFoundActivity::class.java), |
There was a problem hiding this comment.
[Info] ✅ SchemeType 열거형 구조가 깔끔합니다.
CALLVAN과 CALLVAN_CHAT 항목 추가가 기존 패턴을 잘 따르고 있으며, ; 위치도 Kotlin 컨벤션에 맞게 마지막 항목 다음 줄로 이동된 점이 좋습니다. LOST_AND_FOUND에서 불필요하게 존재하던 빈 줄도 함께 정리한 점이 깔끔합니다.
코드 리뷰 종합 의견
전체 평가콜밴팟 알림 구현을 위한 Deep Link/Scheme 처리가 전반적으로 깔끔하게 구현되었습니다. Critical 또는 Major 수준의 버그는 없습니다. 기존 아키텍처 패턴을 잘 따르고 있으며 변경 범위도 적절합니다. 잘 된 점 ✅
지적 사항 요약
추가 확인 권장 사항
// 현재 코드
val postId: Int? = savedStateHandle[EXTRA_POST_ID]
checkNotNull(postId) // null은 걸러내지만 -1은 통과됨
getCallvanPostDetail(postId)정상 플로우(알림 → SchemeActivity → GroupChatActivity)에서는 항상 유효한 val postId = checkNotNull(savedStateHandle.get<Int>(EXTRA_POST_ID)) { "postId must not be null" }
require(postId > 0) { "postId must be positive, got $postId" }전반적으로 잘 구현된 PR입니다. Minor 사항들은 선택적으로 반영하셔도 무방하며, Trivial 항목은 다음 리팩토링 시 함께 처리하셔도 됩니다. |
|
| LaunchedEffect(Unit) { | ||
| if (targetId == -1) return@LaunchedEffect | ||
| navController.navigate(CallvanNavType.CallvanDetail(targetId)) | ||
| } |
There was a problem hiding this comment.
[Major] 화면 회전 시 중복 네비게이션 버그
LaunchedEffect(Unit)은 컴포지션이 새로 시작될 때마다 실행됩니다. rememberNavController()는 백스택 상태를 복원하지만, 화면 회전 후 재구성 시 LaunchedEffect(Unit) 블록이 다시 실행되어 이미 CallvanDetail이 스택에 있는 상태에서 또 navigate를 호출하게 됩니다. 결과적으로 백스택에 CallvanDetail이 중복으로 쌓입니다.
또한, 이 코드에는 더 우아한 기존 패턴이 존재합니다. EXTRA_NAVIGATE_TO_CREATE는 startDestination으로 처리하는 반면, EXTRA_ID만 LaunchedEffect로 처리하는 것은 일관성이 없습니다.
권장 수정 방법:
| LaunchedEffect(Unit) { | |
| if (targetId == -1) return@LaunchedEffect | |
| navController.navigate(CallvanNavType.CallvanDetail(targetId)) | |
| } | |
| var hasNavigated by rememberSaveable { mutableStateOf(false) } | |
| val targetId = intent.getIntExtra(EXTRA_ID, -1) | |
| // Scheme url from backend is not matching with deeplink url in navigation | |
| // Navigate to the correct destination from here instead of relying on URI matching. | |
| LaunchedEffect(Unit) { | |
| if (targetId == -1 || hasNavigated) return@LaunchedEffect | |
| hasNavigated = true | |
| navController.navigate(CallvanNavType.CallvanDetail(targetId)) | |
| } |
또는 기존 startDestination 패턴을 따라 아예 startDestination에서 처리하는 방법도 고려해 보세요.
| val targetId = intent.getIntExtra(EXTRA_ID, -1) | ||
|
|
||
| // Scheme url from backend is not matching with deeplink url in navigation | ||
| // So, let's navigate to correct deeplink from here |
There was a problem hiding this comment.
[Trivial] 프로덕션 코드의 비격식 표현
let's와 같은 비격식 표현은 프로덕션 코드 주석에 적합하지 않습니다.
| // So, let's navigate to correct deeplink from here | |
| // The scheme URL from the backend does not match the deep-link URL defined in the | |
| // navigation graph, so navigate programmatically to the correct destination. |
| LaunchedEffect(Unit) { | ||
| launcher.launch(intent) | ||
| navigator.navigateToGroupChat(context).apply { | ||
| putExtra(EXTRA_POST_ID, it.toRoute<CallvanNavType.CallvanChat>().postId) | ||
| }.let(launcher::launch) | ||
| } |
There was a problem hiding this comment.
[Major] 화면 회전 시 GroupChatActivity 재실행 + 가독성 문제
두 가지 문제가 있습니다:
-
재실행 버그:
LaunchedEffect(Unit)은 Activity 재생성(화면 회전) 시 재실행됩니다. 즉,launcher.launch(intent)가 다시 호출되어GroupChatActivity가 또 실행됩니다. -
암묵적
it캡처: 바깥composable<CallvanNavType.CallvanChat> { ... }람다의it(=NavBackStackEntry)을LaunchedEffect블록 안에서 암묵적으로 캡처하고 있어 가독성이 떨어집니다. 또한.apply { ... }.let(launcher::launch)체이닝은 필요 이상으로 복잡합니다.
권장 수정 방법:
| LaunchedEffect(Unit) { | |
| launcher.launch(intent) | |
| navigator.navigateToGroupChat(context).apply { | |
| putExtra(EXTRA_POST_ID, it.toRoute<CallvanNavType.CallvanChat>().postId) | |
| }.let(launcher::launch) | |
| } | |
| composable<CallvanNavType.CallvanChat> { backStackEntry -> | |
| val navigator = rememberNavigator() | |
| val context = LocalContext.current | |
| val postId = backStackEntry.toRoute<CallvanNavType.CallvanChat>().postId | |
| val launcher = rememberLauncherForActivityResult( | |
| contract = ActivityResultContracts.StartActivityForResult() | |
| ) { | |
| navController.popBackStack() | |
| } | |
| var hasLaunched by rememberSaveable { mutableStateOf(false) } | |
| LaunchedEffect(Unit) { | |
| if (hasLaunched) return@LaunchedEffect | |
| hasLaunched = true | |
| val intent = navigator.navigateToGroupChat(context).apply { | |
| putExtra(EXTRA_POST_ID, postId) | |
| } | |
| launcher.launch(intent) | |
| } | |
| } |
| val postId: Int? = savedStateHandle[EXTRA_POST_ID] | ||
| checkNotNull(postId) |
There was a problem hiding this comment.
[Major] CALLVAN_CHAT 스킴 URL에 postId 누락 시 앱 크래시
SchemeType.CALLVAN_CHAT으로 라우팅될 때, SchemeActivity는 URL에서 ?postId=X 쿼리 파라미터를 추출합니다. 만약 백엔드 알림 URL에 postId가 빠져 있거나 URL 형식이 변경된다면:
getPostIdFromUrl()→null반환.filter { it.second != null }에 의해 필터링됨GroupChatActivity가EXTRA_POST_ID없이 실행됨savedStateHandle[EXTRA_POST_ID]→nullcheckNotNull(postId)→IllegalStateException크래시
checkNotNull은 개발 단계 early-fail에 유용하지만, 프로덕션 환경에서는 사용자가 앱을 강제종료 당하는 경험을 합니다. 최소한 에러 상태를 UI에 표시하고 gracefully 종료하도록 처리해야 합니다.
| val postId: Int? = savedStateHandle[EXTRA_POST_ID] | |
| checkNotNull(postId) | |
| val postId: Int? = savedStateHandle[EXTRA_POST_ID] | |
| if (postId == null) { | |
| postSideEffect(GroupChatSideEffect.FailedToLoadMessages) | |
| return@container | |
| } |
혹은 checkNotNull 유지 시, 해당 케이스를 SchemeActivity에서 사전 검증하여 ID 없이는 GroupChatActivity를 실행하지 않도록 방어하는 로직을 추가하는 것을 권장합니다.
| CALLVAN_CHAT("callvan-chat", GroupChatActivity::class.java) | ||
| ; |
There was a problem hiding this comment.
[Trivial] 세미콜론 위치 스타일 불일치
기존 코드는 마지막 enum entry에 세미콜론을 같은 줄에 붙이는 스타일이었습니다 (LOST_AND_FOUND(...);). 세미콜론을 새 줄로 분리하면 스타일이 일관되지 않습니다.
| CALLVAN_CHAT("callvan-chat", GroupChatActivity::class.java) | |
| ; | |
| CALLVAN("callvan", CallvanActivity::class.java), | |
| CALLVAN_CHAT("callvan-chat", GroupChatActivity::class.java); |
| navigator.navigateTo( | ||
| context = this@SplashActivity, | ||
| type = Pair(EXTRA_TYPE, type), | ||
| *arrayOf( | ||
| Pair(EXTRA_ID, targetId), | ||
| Pair(EXTRA_BOARD_ID, targetBoardId), | ||
| Pair(EXTRA_ARTICLE_ID, targetArticleId), | ||
| Pair(EXTRA_CHAT_ROOM_ID, targetChatId), | ||
| Pair(EXTRA_CLUB_ID, targetClubId), | ||
| Pair(EXTRA_EVENT_ID, targetEventId) | ||
| ) | ||
| Pair(EXTRA_ID, targetId), | ||
| Pair(EXTRA_BOARD_ID, targetBoardId), | ||
| Pair(EXTRA_ARTICLE_ID, targetArticleId), | ||
| Pair(EXTRA_CHAT_ROOM_ID, targetChatId), | ||
| Pair(EXTRA_CLUB_ID, targetClubId), | ||
| Pair(EXTRA_EVENT_ID, targetEventId), | ||
| Pair(EXTRA_POST_ID, targetPostId) | ||
| ) |
There was a problem hiding this comment.
[Info] Spread operator 제거 - 좋은 개선입니다
*arrayOf(...) 스프레드 연산자를 제거하고 vararg 인자를 직접 전달하도록 변경한 것은 Detekt 경고 해결뿐 아니라 불필요한 배열 객체 생성을 막아 성능상으로도 좋은 개선입니다.
| @@ -42,7 +43,7 @@ class GroupChatViewModel @Inject constructor( | |||
| ) : ViewModel(), ContainerHost<GroupChatState, GroupChatSideEffect> { | |||
|
|
|||
| override val container = container<GroupChatState, GroupChatSideEffect>(GroupChatState()) { | |||
There was a problem hiding this comment.
[Major] postId가 -1일 때 잘못된 API 호출 발생
이번 PR에서 CALLVAN_CHAT 스킴 타입이 새로 추가되어 GroupChatActivity가 푸시 알림을 통해서도 도달 가능해졌습니다. 이 경로에서 심각한 버그가 발생할 수 있습니다.
문제 흐름:
- 백엔드가
postId파라미터 없이callvan-chat스킴 알림을 전송 SplashActivity에서intent.getIntExtra(EXTRA_POST_ID, -1)→-1반환-1이GroupChatActivity의 Intent extra로 전달됨savedStateHandle[EXTRA_POST_ID]→-1반환 (null이 아님!)checkNotNull(-1)통과 →getCallvanPostDetail(-1)호출 → 잘못된 API 요청
SavedStateHandle에서 Int 타입의 키는 값이 -1이어도 null이 아니므로 checkNotNull이 통과됩니다.
| override val container = container<GroupChatState, GroupChatSideEffect>(GroupChatState()) { | |
| val postId: Int = checkNotNull(savedStateHandle.get<Int>(EXTRA_POST_ID)) { | |
| "postId must not be null" | |
| } | |
| require(postId > 0) { "Invalid postId: $postId" } | |
| getCallvanPostDetail(postId) |
혹은 유효하지 않은 postId일 때 Activity를 종료하는 방어 로직이 필요합니다.
| LaunchedEffect(Unit) { | ||
| launcher.launch(intent) | ||
| navigator.navigateToGroupChat(context).apply { | ||
| putExtra(EXTRA_POST_ID, it.toRoute<CallvanNavType.CallvanChat>().postId) |
There was a problem hiding this comment.
[Minor] apply {} 블록 내 it 의 의미가 불명확함
apply {} 블록 안에서 this는 Intent 수신자를 가리키지만, it은 외부 composable<CallvanNavType.CallvanChat> { } 람다의 암시적 파라미터인 NavBackStackEntry를 캡처합니다. Kotlin 문법상 동작은 맞지만, it이 Intent인지 NavBackStackEntry인지 혼동을 일으킬 수 있어 유지보수 시 버그 위험이 있습니다.
| putExtra(EXTRA_POST_ID, it.toRoute<CallvanNavType.CallvanChat>().postId) | |
| LaunchedEffect(Unit) { | |
| val postId = it.toRoute<CallvanNavType.CallvanChat>().postId | |
| navigator.navigateToGroupChat(context) | |
| .apply { putExtra(EXTRA_POST_ID, postId) } | |
| .let(launcher::launch) | |
| } |
또는 외부 람다 파라미터를 명시적으로 이름 지정하는 것을 권장합니다:
composable<CallvanNavType.CallvanChat> { backStackEntry ->
...
LaunchedEffect(Unit) {
navigator.navigateToGroupChat(context).apply {
putExtra(EXTRA_POST_ID, backStackEntry.toRoute<CallvanNavType.CallvanChat>().postId)
}.let(launcher::launch)
}
}
| RebrandKoinTheme { | ||
| val navController = rememberNavController() | ||
|
|
||
| val targetId = intent.getIntExtra(EXTRA_ID, -1) |
There was a problem hiding this comment.
[Minor] remember 없이 recomposition마다 intent extra를 재읽음
targetId가 setContent {} 내부에서 remember 없이 읽히면, recomposition이 발생할 때마다 intent.getIntExtra()가 재호출됩니다. 이 값은 Activity 생명주기 동안 변하지 않으므로 remember로 캐싱하는 것이 Compose 관례에 맞습니다.
| val targetId = intent.getIntExtra(EXTRA_ID, -1) | |
| val targetId = remember { intent.getIntExtra(EXTRA_ID, -1) } |
|
|
||
| // Scheme url from backend is not matching with deeplink url in navigation | ||
| // So, let's navigate to correct deeplink from here | ||
| LaunchedEffect(Unit) { |
There was a problem hiding this comment.
[Minor] LaunchedEffect(Unit) 키 — onNewIntent 시 재탐색 불가
Unit을 키로 사용하면 Composition이 처음 진입할 때 단 한 번만 실행됩니다. 만약 이 Activity가 android:launchMode="singleTop" 또는 FLAG_ACTIVITY_SINGLE_TOP으로 재사용되어 onNewIntent()가 호출된다면, 새로운 targetId로 네비게이션이 트리거되지 않습니다.
현재 구조에서 onNewIntent를 처리하지 않는다면 이 패턴은 괜찮지만, 주석이나 TODO로 이 제약을 명시해두는 것을 권장합니다.
| LaunchedEffect(Unit) { | |
| // NOTE: LaunchedEffect(Unit) intentionally runs only once per composition entry. | |
| // If this Activity needs to handle onNewIntent (e.g. singleTop), use | |
| // rememberUpdatedState + LaunchedEffect(targetId) instead. | |
| // TODO: Remove this workaround once backend deeplink scheme is aligned with nav graph. | |
| LaunchedEffect(Unit) { |
| LOST_AND_FOUND("articles/lost-item", LostAndFoundActivity::class.java); | ||
| LOST_AND_FOUND("articles/lost-item", LostAndFoundActivity::class.java), | ||
| CALLVAN("callvan", CallvanActivity::class.java), | ||
| CALLVAN_CHAT("callvan-chat", GroupChatActivity::class.java) |
There was a problem hiding this comment.
[Trivial] GroupChatActivity를 SchemeType에 직접 노출하는 구조 검토 필요
CALLVAN_CHAT("callvan-chat", GroupChatActivity::class.java) 항목은 GroupChatActivity가 callvan 기능 외부(:koin 앱 모듈)에서 직접 참조됨을 의미합니다. GroupChatActivity는 callvan 채팅 전용 Activity임에도 불구하고 :feature:chat 모듈 내부에 있어, 모듈 경계가 약해질 수 있습니다.
중장기적으로는 CallvanActivity를 통해 내비게이션을 통합하거나(CALLVAN으로 진입 후 내부에서 라우팅), 또는 GroupChatActivity를 :feature:callvan 모듈로 이동하는 방안을 검토해볼 수 있습니다. 이번 PR의 직접적인 변경사항은 아니지만, 현재 코드의 CallvanActivity.kt 주석("Scheme url from backend is not matching")과 함께 아키텍처 관점에서 향후 개선 포인트로 남겨둘 만합니다.
| Pair(EXTRA_CHAT_ROOM_ID, targetChatId), | ||
| Pair(EXTRA_CLUB_ID, targetClubId), | ||
| Pair(EXTRA_EVENT_ID, targetEventId), | ||
| Pair(EXTRA_POST_ID, targetPostId) |
There was a problem hiding this comment.
[Info] SpreadOperator 제거 — 잘 처리했습니다!
*arrayOf(...) 대신 varargs를 직접 나열하는 방식으로 변경하여 Detekt의 SpreadOperator 경고를 제거한 것은 올바른 방향입니다. SpreadOperator는 내부적으로 배열 복사가 발생해 불필요한 메모리 할당이 생기므로, 이처럼 인수를 직접 나열하는 것이 성능상 더 좋습니다.
| private fun getArticleIdFromUrl(url: String): Int? { | ||
| return Uri.parse(url).getQueryParameter("articleId")?.toIntOrNull() | ||
| return url.toUri().getQueryParameter("articleId")?.toIntOrNull() | ||
| } |
There was a problem hiding this comment.
[Info] Uri.parse() → url.toUri() 일괄 마이그레이션 — 잘 처리했습니다!
androidx.core.net.toUri() KTX 확장함수로의 일괄 전환은 Kotlin 관용적 스타일에 부합합니다. 기능상 동일하지만 코드의 일관성이 향상되었습니다.
2 participants
PR 개요
PR 체크리스트
작업사항
작업사항의 상세한 설명
GroupChatActivity의createIntent를 공통buildIntent로 수정했습니다.논의 사항
스크린샷
추가내용