Conversation
Walkthrough도착 감지를 수행하는 새로운 포그라운드 서비스 Changes
Sequence DiagramsequenceDiagram
participant User
participant HomeScreen
participant ArrivalMonitorService
participant LocationProvider
participant MainActivity
User->>HomeScreen: 알람(목적지) 등록
HomeScreen->>ArrivalMonitorService: startForegroundService(Intent{destLat,destLng})
Note over ArrivalMonitorService: 포그라운드 알림 생성 및 startForeground()
ArrivalMonitorService->>LocationProvider: requestLocationUpdates(LOW_POWER)
loop 위치 업데이트 수신
LocationProvider->>ArrivalMonitorService: onLocationResult(location)
ArrivalMonitorService->>ArrivalMonitorService: 거리 계산 (현재 ↔ 목적지)
alt within 30m
ArrivalMonitorService->>ArrivalMonitorService: vibrate(), notify()
ArrivalMonitorService->>MainActivity: sendBroadcast(ACTION_ARRIVAL)
ArrivalMonitorService->>ArrivalMonitorService: stopSelf() / removeLocationUpdates
MainActivity->>User: 도착 다이얼로그 표시
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.kt📄 CodeRabbit inference engine (GEMINI.md)
Files:
**/*Service.kt📄 CodeRabbit inference engine (GEMINI.md)
Files:
🧠 Learnings (1)📚 Learning: 2025-11-30T06:06:10.920ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 5
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/depromeet/team6/data/background/ArrivalMonitorService.kt(1 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt(3 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.kt(5 hunks)app/src/main/res/values/strings.xml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-18T12:10:51.782Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-10-18T12:10:51.782Z
Learning: Use Coroutines and Flow for asynchronous work and reactive streams
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-10-18T12:10:51.782Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-10-18T12:10:51.782Z
Learning: Separate layers: UI with Jetpack Compose subscribing to ViewModel state; Domain as pure Kotlin with UseCases; Data using repositories abstracting sources
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-10-18T12:10:51.782Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-10-18T12:10:51.782Z
Learning: Use AAC ViewModel and Navigation for lifecycle-aware state and app navigation
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-10-18T12:10:51.782Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-10-18T12:10:51.782Z
Learning: Adopt Clean Architecture with MVI and unidirectional data flow (Intent -> ViewModel -> State -> UI)
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI
🔇 Additional comments (2)
app/src/main/res/values/strings.xml (1)
254-260: LGTM!도착 모니터링 기능을 위한 문자열 리소스가 올바르게 추가되었습니다.
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.kt (1)
227-254: BroadcastReceiver 구현 양호
DisposableEffect를 사용한 생명주기 인식 등록/해제와RECEIVER_NOT_EXPORTED플래그 사용이 적절합니다. 내부 앱 통신에 올바른 보안 패턴을 따르고 있습니다.
| private var destinationLatLng: LatLng? = null | ||
| private var hasArrived = false | ||
| private var vibrator: Vibrator? = null | ||
| private val timerScope = CoroutineScope(Dispatchers.Default) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
사용되지 않는 timerScope 제거 필요
timerScope가 선언되고 onDestroy()에서 취소되지만, 어디에서도 코루틴을 실행하지 않습니다. 사용하지 않는 코드는 제거하세요.
- private val timerScope = CoroutineScope(Dispatchers.Default)그리고 onDestroy()에서도 제거:
override fun onDestroy() {
fusedLocationClient?.removeLocationUpdates(locationCallback)
- timerScope.cancel()
super.onDestroy()
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
app/src/main/java/com/depromeet/team6/data/background/ArrivalMonitorService.kt
around line 37, the private val timerScope = CoroutineScope(Dispatchers.Default)
is declared but never used; remove the unused timerScope declaration and also
remove its cancellation from onDestroy() so there is no dangling unused
coroutine scope; ensure there are no other references to timerScope before
deleting both the field and the onDestroy cancellation call.
| override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { | ||
| val lat = intent?.getDoubleExtra(EXTRA_DEST_LAT, 0.0) ?: 0.0 | ||
| val lng = intent?.getDoubleExtra(EXTRA_DEST_LNG, 0.0) ?: 0.0 | ||
| destinationLatLng = LatLng(lat, lng) | ||
|
|
||
| startLocationTracking() | ||
| return START_STICKY | ||
| } |
There was a problem hiding this comment.
START_STICKY 사용 시 데이터 손실 위험
서비스가 시스템에 의해 종료 후 재시작되면 onStartCommand가 null intent로 호출됩니다. 이 경우 destinationLatLng가 (0.0, 0.0)으로 설정되어 대서양 한가운데를 목적지로 모니터링하게 됩니다.
override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int {
val lat = intent?.getDoubleExtra(EXTRA_DEST_LAT, 0.0) ?: 0.0
val lng = intent?.getDoubleExtra(EXTRA_DEST_LNG, 0.0) ?: 0.0
+
+ // 유효하지 않은 좌표인 경우 서비스 중지
+ if (lat == 0.0 && lng == 0.0) {
+ stopSelf()
+ return START_NOT_STICKY
+ }
+
destinationLatLng = LatLng(lat, lng)
startLocationTracking()
- return START_STICKY
+ return START_NOT_STICKY
}📝 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.
| override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { | |
| val lat = intent?.getDoubleExtra(EXTRA_DEST_LAT, 0.0) ?: 0.0 | |
| val lng = intent?.getDoubleExtra(EXTRA_DEST_LNG, 0.0) ?: 0.0 | |
| destinationLatLng = LatLng(lat, lng) | |
| startLocationTracking() | |
| return START_STICKY | |
| } | |
| override fun onStartCommand(intent: Intent?, flags: Int, startId: Int): Int { | |
| val lat = intent?.getDoubleExtra(EXTRA_DEST_LAT, 0.0) ?: 0.0 | |
| val lng = intent?.getDoubleExtra(EXTRA_DEST_LNG, 0.0) ?: 0.0 | |
| // 유효하지 않은 좌표인 경우 서비스 중지 | |
| if (lat == 0.0 && lng == 0.0) { | |
| stopSelf() | |
| return START_NOT_STICKY | |
| } | |
| destinationLatLng = LatLng(lat, lng) | |
| startLocationTracking() | |
| return START_NOT_STICKY | |
| } |
| private fun startLocationTracking() { | ||
| if (checkSelfPermission( | ||
| this, | ||
| Manifest.permission.ACCESS_FINE_LOCATION | ||
| ) != PackageManager.PERMISSION_GRANTED && checkSelfPermission( | ||
| this, | ||
| Manifest.permission.ACCESS_COARSE_LOCATION | ||
| ) != PackageManager.PERMISSION_GRANTED | ||
| ) { | ||
| return | ||
| } |
There was a problem hiding this comment.
권한 거부 시 서비스가 계속 실행됨
위치 권한이 없으면 startLocationTracking()이 조용히 반환되지만, 서비스는 포그라운드에서 계속 실행됩니다. 리소스 낭비이며 사용자에게 혼란을 줄 수 있습니다.
private fun startLocationTracking() {
if (checkSelfPermission(
this,
Manifest.permission.ACCESS_FINE_LOCATION
) != PackageManager.PERMISSION_GRANTED && checkSelfPermission(
this,
Manifest.permission.ACCESS_COARSE_LOCATION
) != PackageManager.PERMISSION_GRANTED
) {
+ stopSelf()
return
}📝 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.
| private fun startLocationTracking() { | |
| if (checkSelfPermission( | |
| this, | |
| Manifest.permission.ACCESS_FINE_LOCATION | |
| ) != PackageManager.PERMISSION_GRANTED && checkSelfPermission( | |
| this, | |
| Manifest.permission.ACCESS_COARSE_LOCATION | |
| ) != PackageManager.PERMISSION_GRANTED | |
| ) { | |
| return | |
| } | |
| private fun startLocationTracking() { | |
| if (checkSelfPermission( | |
| this, | |
| Manifest.permission.ACCESS_FINE_LOCATION | |
| ) != PackageManager.PERMISSION_GRANTED && checkSelfPermission( | |
| this, | |
| Manifest.permission.ACCESS_COARSE_LOCATION | |
| ) != PackageManager.PERMISSION_GRANTED | |
| ) { | |
| stopSelf() | |
| return | |
| } |
🤖 Prompt for AI Agents
In
app/src/main/java/com/depromeet/team6/data/background/ArrivalMonitorService.kt
around lines 95 to 105, startLocationTracking() currently returns silently when
location permissions are missing which leaves the foreground service running;
update the method to explicitly stop the service when permissions are not
granted by calling stopForeground(true) and stopSelf(), and (optionally) post a
notification or broadcast to prompt the UI to request permissions before
restarting the service; ensure these cleanup calls run on the service main
thread and remove any further location setup when permissions are absent.
| val notification = NotificationCompat.Builder(this, "ARRIVAL_MONITOR_CHANNEL") | ||
| .setSmallIcon(R.drawable.ic_atcha_logo) | ||
| .setContentTitle( | ||
| getString(R.string.notification_arrival_guide_finish_title) | ||
| ) | ||
| .setContentText( | ||
| getString(R.string.notification_arrival_near_destination_text) | ||
| ) | ||
| .setContentIntent(pendingIntent) | ||
| .setAutoCancel(true) | ||
| .build() |
There was a problem hiding this comment.
알림 채널 ID 불일치 - 알림이 표시되지 않음
onArrived()에서 알림 생성 시 하드코딩된 채널 ID "ARRIVAL_MONITOR_CHANNEL"을 사용하고 있지만, 실제 생성된 채널 ID는 ATCHA_ARRIVAL_MONITOR_CHANNEL_ID ("Arrival_Monitor_Channel")입니다. Android 8.0 이상에서 알림이 표시되지 않습니다.
- val notification = NotificationCompat.Builder(this, "ARRIVAL_MONITOR_CHANNEL")
+ val notification = NotificationCompat.Builder(this, ATCHA_ARRIVAL_MONITOR_CHANNEL_ID)
.setSmallIcon(R.drawable.ic_atcha_logo)📝 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 notification = NotificationCompat.Builder(this, "ARRIVAL_MONITOR_CHANNEL") | |
| .setSmallIcon(R.drawable.ic_atcha_logo) | |
| .setContentTitle( | |
| getString(R.string.notification_arrival_guide_finish_title) | |
| ) | |
| .setContentText( | |
| getString(R.string.notification_arrival_near_destination_text) | |
| ) | |
| .setContentIntent(pendingIntent) | |
| .setAutoCancel(true) | |
| .build() | |
| val notification = NotificationCompat.Builder(this, ATCHA_ARRIVAL_MONITOR_CHANNEL_ID) | |
| .setSmallIcon(R.drawable.ic_atcha_logo) | |
| .setContentTitle( | |
| getString(R.string.notification_arrival_guide_finish_title) | |
| ) | |
| .setContentText( | |
| getString(R.string.notification_arrival_near_destination_text) | |
| ) | |
| .setContentIntent(pendingIntent) | |
| .setAutoCancel(true) | |
| .build() |
🤖 Prompt for AI Agents
In
app/src/main/java/com/depromeet/team6/data/background/ArrivalMonitorService.kt
around lines 148 to 158, the NotificationCompat.Builder uses the hardcoded
channel ID "ARRIVAL_MONITOR_CHANNEL" whereas the actual created channel ID
constant is ATCHA_ARRIVAL_MONITOR_CHANNEL_ID ("Arrival_Monitor_Channel");
replace the hardcoded string with the correct channel ID constant
(ATCHA_ARRIVAL_MONITOR_CHANNEL_ID) so the notification is posted to the existing
channel and will appear on Android 8.0+.
| if (homeUiState.isAlarmRegistered) { | ||
| val context = LocalContext.current | ||
| val destination = homeUiState.destinationPoint | ||
|
|
||
| Intent(context, ArrivalMonitorService::class.java).apply { | ||
| putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat) | ||
| putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon) | ||
| }.also { | ||
| ContextCompat.startForegroundService(context, it) | ||
| } |
There was a problem hiding this comment.
Compose 부작용(Side Effect) 규칙 위반 - 서비스가 매 리컴포지션마다 시작됨
포그라운드 서비스를 Composable 함수 본문에서 직접 시작하고 있습니다. isAlarmRegistered가 true인 동안 리컴포지션이 발생할 때마다 startForegroundService가 호출됩니다. 이는 예상치 못한 동작과 잠재적인 성능 문제를 야기할 수 있습니다.
LaunchedEffect로 감싸서 조건 변경 시에만 실행되도록 수정하세요:
if (homeUiState.isAlarmRegistered) {
val context = LocalContext.current
val destination = homeUiState.destinationPoint
- Intent(context, ArrivalMonitorService::class.java).apply {
- putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat)
- putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon)
- }.also {
- ContextCompat.startForegroundService(context, it)
- }
+ LaunchedEffect(destination.lat, destination.lon) {
+ Intent(context, ArrivalMonitorService::class.java).apply {
+ putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat)
+ putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon)
+ }.also {
+ ContextCompat.startForegroundService(context, it)
+ }
+ }
AfterRegisterMap(🤖 Prompt for AI Agents
In app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
around lines 624-633, the foreground service is started directly in the
Composable body causing startForegroundService to run on every recomposition
while isAlarmRegistered is true; move this logic into a LaunchedEffect keyed on
isAlarmRegistered (and destination if it can change) so the Intent creation and
ContextCompat.startForegroundService call only execute when isAlarmRegistered
becomes true (and/or when destination changes) rather than on every
recomposition.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt (1)
646-655: Composable 본문에서 ForegroundService 직접 시작 → 리컴포지션마다 반복 호출 위험
homeUiState.isAlarmRegistered가true인 동안 이 블록이 컴포지션에 남아 있기 때문에, 리컴포지션이 발생할 때마다startForegroundService가 다시 호출될 수 있습니다. 이는:
- 서비스
onStartCommand가 불필요하게 여러 번 호출될 수 있고- 위치 갱신/알림 로직이 중복 실행되며
- 배터리·자원 사용량 증가 및 예측하기 어려운 동작을 유발할 수 있습니다.
MVI/UDF 및 Compose side-effect 규칙 관점에서도, 이런 작업은 Composable 본문이 아니라
LaunchedEffect같은 side-effect 블록으로 분리하는 것이 맞습니다. 과거 리뷰에서도 동일 지적이 있었는데 아직 반영이 안 된 상태입니다.
isAlarmRegistered == true상태로 전환될 때(또는 목적지 좌표 변경 시)에만 서비스가 1회 시작되도록 아래처럼LaunchedEffect로 감싸는 방향을 권장합니다:- if (homeUiState.isAlarmRegistered) { - val context = LocalContext.current - val destination = homeUiState.destinationPoint - - Intent(context, ArrivalMonitorService::class.java).apply { - putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat) - putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon) - }.also { - ContextCompat.startForegroundService(context, it) - } - + if (homeUiState.isAlarmRegistered) { + val context = LocalContext.current + val destination = homeUiState.destinationPoint + + LaunchedEffect(destination.lat, destination.lon) { + Intent(context, ArrivalMonitorService::class.java).apply { + putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat) + putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon) + }.also { + ContextCompat.startForegroundService(context, it) + } + } + AfterRegisterMap(이렇게 하면 상태 변화(목적지/알람 등록)로 인해 컴포지션 트리에 이 블록이 진입할 때만 서비스가 시작되어, 불필요한 반복 호출을 막을 수 있습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt(3 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.kt(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.kt
📄 CodeRabbit inference engine (GEMINI.md)
**/*.kt: Use 100% Kotlin for all code implementation
Implement Clean Architecture with MVI (Model-View-Intent) pattern using single-direction data flow (UDF) where UI events flow through ViewModel to the data layer, and processed State results flow back to UI
Use Coroutines and Flow for asynchronous operations and reactive data streams
Use Hilt for dependency injection to manage dependencies between layers and reduce coupling between components
Apply Proguard and R8 code obfuscation and optimization for release builds to ensure security and performance
Maintain consistent code style using Ktlint plugin for Kotlin code formatting
Files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
🧠 Learnings (6)
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*Service.kt : Use BackgroundService and BroadcastReceiver for background processing tasks such as push notifications and alarms
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*Compose.kt : Use Jetpack Compose with declarative UI to construct UI components and handle user interactions for State subscription from ViewModel
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*.kt : Use Hilt for dependency injection to manage dependencies between layers and reduce coupling between components
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*ViewModel.kt : Use AAC ViewModel for state management and to handle the single-direction data flow (UDF) pattern between UI Layer and Domain Layer
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*.kt : Implement Clean Architecture with MVI (Model-View-Intent) pattern using single-direction data flow (UDF) where UI events flow through ViewModel to the data layer, and processed State results flow back to UI
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*.kt : Use Coroutines and Flow for asynchronous operations and reactive data streams
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.kt
Show resolved
Hide resolved
…ithub.com/depromeet/16th-team6-Android into feat/stop-navigation-near-destination#247
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterSheet.kt (2)
38-62: 사용되지 않는 파라미터 및 주석 처리된 코드 정리 필요
onRefreshClick,onTimerFinished,onHomeExpectDepartureTimeClick파라미터가 함수 시그니처에 남아있지만 실제로 사용되지 않습니다. 또한timerFinish파라미터도 대부분의 관련 로직이 주석 처리되어 제한적으로만 사용됩니다.주석 처리된 코드를 프로덕션에 남겨두면 코드 가독성이 저하되고 유지보수가 어려워집니다. 이 코드들이 더 이상 필요 없다면 완전히 제거하고, 필요하다면 주석을 해제하여 활성화하세요.
fun AfterRegisterSheet( timerFinish: Boolean, -// isConfirmed: Boolean, afterUserDeparted: Boolean, transportType: TransportType, transportationNumber: Int, transportationName: String, timeToLeave: String, boardingTime: String, homeArrivedTime: String, startLocation: String, destination: String, deleteAlarmConfirmed: () -> Unit = {}, dismissDialog: () -> Unit = {}, busStationLeft: Int, onCourseTextClick: () -> Unit, onFinishClick: () -> Unit, onCourseDetailClick: (String) -> Unit, - onRefreshClick: () -> Unit, - onTimerFinished: () -> Unit = {}, onIconClick: () -> Unit = {}, onHomeDepartureTimeClick: () -> Unit = {}, - onHomeExpectDepartureTimeClick: () -> Unit = {}, modifier: Modifier = Modifier )
201-209: 조건 분기 로직 단순화 가능
timerFinish와afterUserDeparted조합에 따른 분기가 있지만, 주석 처리된isConfirmed관련 분기가 제거되면서 로직이 더 단순해질 수 있습니다. 현재timerFinish가false인 경우와true인 경우 모두afterUserDeparted가true면 다른 amplitude 이벤트만 전송하는 구조입니다.app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterSpeechBubble.kt (1)
17-46: 미사용 파라미터와 비활성화된 로직 정리 필요
prefixText,emphasisText,suffixText,showSpeechBubble파라미터가 모두 사용되지 않습니다. 현재showSpeechBubble이true일 때 단순히Spacer만 렌더링하며, 이는 의도한 동작이 아닌 것으로 보입니다.이 컴포넌트가 더 이상 필요 없다면 완전히 제거하고, 리팩토링 중이라면 주석 처리 대신 명확한 TODO 또는 완전한 구현으로 대체하세요.
두 가지 옵션 중 선택:
옵션 1: 컴포넌트 단순화 (말풍선 불필요 시)
@Composable fun CharacterSpeechBubble( - prefixText: String, modifier: Modifier = Modifier, - emphasisText: String? = null, - suffixText: String? = null, onClick: () -> Unit = {}, - showSpeechBubble: Boolean = true ) { Column( modifier = modifier.noRippleClickable { onClick() } ) { - if (showSpeechBubble) { - Spacer(modifier = Modifier.height(2.dp)) - } - Image( imageVector = ImageVector.vectorResource(R.drawable.ic_all_atcha_character), contentDescription = stringResource(R.string.all_atcha_character) ) } }옵션 2: 말풍선 로직 복원 또는 새 API로 마이그레이션
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterLottieSpeechBubble.kt (1)
44-91: 사용되지 않는 상태 및 불필요한 코루틴 실행
isTopSpeechBubbleVisible,isBottomSpeechBubbleVisible상태와 관련된 모든LaunchedEffect로직(Lines 56-91)이 여전히 실행되지만, 해당 상태를 사용하는 UI 코드는 모두 주석 처리되었습니다. 이로 인해:
- 불필요한 코루틴 delay 실행 (최대 4000ms+)
- 사용되지 않는 상태 리컴포지션
- 불필요한 메모리 사용
UI 코드가 비활성화된 상태라면 관련 상태와 LaunchedEffect도 함께 제거하거나 주석 처리하세요:
- var isTopSpeechBubbleVisible by remember { mutableStateOf(false) } - var isBottomSpeechBubbleVisible by remember { mutableStateOf(false) } - - var speechBubbleTrigger by remember { mutableStateOf(0) } - - val hasAnyText = !prefixText.isBlank() || - !emphasisText.isNullOrBlank() || - !suffixText.isNullOrBlank() || - !topPrefixText.isNullOrBlank() || - !topEmphasisText.isNullOrBlank() || - !topSuffixText.isNullOrBlank() - - LaunchedEffect(externalTrigger) { ... } - LaunchedEffect(speechBubbleTrigger, showSpeechBubble, hasAnyText) { ... } - LaunchedEffect(Unit) { ... }app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.kt (1)
74-105: Race condition in init block between async prefs loading and departure state check, plus unused initial speech calculationThe init block has two significant issues:
Async execution order race condition (lines 81-82)
loadAlarmAndCourseInfoFromPrefs()is not a suspend function—it internally launches a newviewModelScope.launch { ... }and returns immediately. This meansloadUserDepartureState()executes right after, likely before theLoadFirstTransportationevent (set at line 562) is processed into state. WhenloadUserDepartureState()checks at line 482:if (userDeparture && currentState.firtTransportTation == TransportType.BUS) { getBusArrival() }
currentState.firtTransportTationis still at its default value (TransportType.WALK), sogetBusArrival()may never be called even when the first transport is actually BUS.Make
loadAlarmAndCourseInfoFromPrefs()asuspendfunction to ensure prefs/course data load sequentially before checking departure state:
- fun loadAlarmAndCourseInfoFromPrefs() {
viewModelScope.launch {
- suspend fun loadAlarmAndCourseInfoFromPrefs() {
// ...
}Then call it as a regular await in init: ```diff loadAlarmAndCourseInfoFromPrefs() val initialSpeech = if (loadUserDepartureState()) { ... }
Initial speech message is computed but never applied to state (lines 82-96)
initialSpeechis calculated based on departure state and taxi cost, but thesetStateblock (lines 98-103) only copiescurrentLocationandloadState. ThecharacterMessagesfield remains at its default emptySpeechRequest(emptyList()), so the initial speech bubble never appears.Add
characterMessages = initialSpeechto thesetStatecall:setState { copy( currentLocation = currentLocation,
loadState = LoadState.Success
loadState = LoadState.Success,characterMessages = initialSpeech ) }
♻️ Duplicate comments (1)
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt (1)
658-667: Compose 부작용(Side Effect) 규칙 위반 - 서비스가 매 리컴포지션마다 재시작됨이 코드는 Composable 본문에서 직접
startForegroundService를 호출하고 있습니다.isAlarmRegistered가 true인 동안 UI가 리컴포지션될 때마다 서비스가 반복적으로 시작됩니다. 이는 불필요한 리소스 소비와 예측 불가능한 동작을 야기합니다.
LaunchedEffect를 사용하여 조건 변경 시에만 실행되도록 수정해야 합니다:if (homeUiState.isAlarmRegistered) { val context = LocalContext.current val destination = homeUiState.destinationPoint + LaunchedEffect(destination.lat, destination.lon) { - Intent(context, ArrivalMonitorService::class.java).apply { - putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat) - putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon) - }.also { - ContextCompat.startForegroundService(context, it) - } + Intent(context, ArrivalMonitorService::class.java).apply { + putExtra(ArrivalMonitorService.EXTRA_DEST_LAT, destination.lat) + putExtra(ArrivalMonitorService.EXTRA_DEST_LNG, destination.lon) + }.also { + ContextCompat.startForegroundService(context, it) + } + } AfterRegisterMap(
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
app/release/.DS_Storeis excluded by!**/.DS_Store
📒 Files selected for processing (15)
GEMINI.md(1 hunks)app/build.gradle.kts(1 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubble.kt(3 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubbleTest.kt(1 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeContract.kt(3 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt(20 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.kt(6 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterMap.kt(3 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterSheet.kt(6 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterLottieSpeechBubble.kt(1 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterSpeechBubble.kt(1 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CourseTextButton.kt(1 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/home/component/MapView.kt(2 hunks)app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.kt(5 hunks)app/src/main/res/values/strings.xml(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.kt
📄 CodeRabbit inference engine (GEMINI.md)
**/*.kt: Use 100% Kotlin for all code implementation
Implement Clean Architecture with MVI (Model-View-Intent) pattern using single-direction data flow (UDF) where UI events flow through ViewModel to the data layer, and processed State results flow back to UI
Use Coroutines and Flow for asynchronous operations and reactive data streams
Use Hilt for dependency injection to manage dependencies between layers and reduce coupling between components
Apply Proguard and R8 code obfuscation and optimization for release builds to ensure security and performance
Maintain consistent code style using Ktlint plugin for Kotlin code formatting
Files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterLottieSpeechBubble.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/CourseTextButton.ktapp/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterMap.ktapp/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubble.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeContract.ktapp/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubbleTest.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterSpeechBubble.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterSheet.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/MapView.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
**/*ViewModel.kt
📄 CodeRabbit inference engine (GEMINI.md)
Use AAC ViewModel for state management and to handle the single-direction data flow (UDF) pattern between UI Layer and Domain Layer
Files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.kt
🧠 Learnings (6)
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*Compose.kt : Use Jetpack Compose with declarative UI to construct UI components and handle user interactions for State subscription from ViewModel
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterLottieSpeechBubble.ktapp/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubble.ktapp/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubbleTest.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterSpeechBubble.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterSheet.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/component/MapView.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*Service.kt : Use BackgroundService and BroadcastReceiver for background processing tasks such as push notifications and alarms
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*ViewModel.kt : Use AAC ViewModel for state management and to handle the single-direction data flow (UDF) pattern between UI Layer and Domain Layer
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/main/MainActivity.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.ktapp/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*.kt : Use Hilt for dependency injection to manage dependencies between layers and reduce coupling between components
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*.kt : Implement Clean Architecture with MVI (Model-View-Intent) pattern using single-direction data flow (UDF) where UI events flow through ViewModel to the data layer, and processed State results flow back to UI
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
📚 Learning: 2025-11-30T06:06:10.920Z
Learnt from: CR
Repo: depromeet/16th-team6-Android PR: 0
File: GEMINI.md:0-0
Timestamp: 2025-11-30T06:06:10.920Z
Learning: Applies to **/*.kt : Use Coroutines and Flow for asynchronous operations and reactive data streams
Applied to files:
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
🧬 Code graph analysis (4)
app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubbleTest.kt (2)
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.kt (1)
onCharacterClick(403-405)app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubble.kt (1)
AtchaSpeechBubble(25-71)
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.kt (6)
app/src/main/java/com/depromeet/team6/presentation/ui/coursesearch/CourseSearchViewModel.kt (1)
getTaxiCost(274-290)app/src/main/java/com/depromeet/team6/data/dataremote/service/TaxiCostService.kt (2)
getTaxiCost(10-18)getTaxiCost(11-17)app/src/main/java/com/depromeet/team6/domain/repository/TaxiCostRepository.kt (2)
getTaxiCost(6-16)getTaxiCost(7-7)app/src/main/java/com/depromeet/team6/data/repositoryimpl/TaxiCostRepositoryImpl.kt (1)
getTaxiCost(25-40)app/src/main/java/com/depromeet/team6/presentation/util/base/BaseViewModel.kt (2)
setState(39-41)setEvent(43-45)app/src/main/java/com/depromeet/team6/data/repositoryimpl/TransitsRepositoryImpl.kt (1)
getBusArrival(29-42)
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/AfterRegisterSheet.kt (1)
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/TimeText.kt (1)
TimeText(15-57)
app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt (1)
app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubbleTest.kt (1)
AtchaSpeechCharacter(51-177)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI
🔇 Additional comments (4)
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CourseTextButton.kt (1)
31-36: 터치 영역 관점에서 더 안전한 변경입니다
noRippleClickable를padding보다 먼저 적용해서 패딩 포함 전체 Row 영역이 클릭 가능해졌고, 이는 사용자 실수 터치 감소와 접근성 측면에서 바람직합니다. 성능·아키텍처·보안 측면에서도 추가 리스크는 없어 보입니다.app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt (3)
462-471: 화면 정리 로직 적절함
DisposableEffect를 사용하여 화면이 사라질 때isMapReady상태를 초기화하는 것은 올바른 패턴입니다.
836-849: 맵 준비 상태에 따른 조건부 렌더링 적절함
isMapReady상태에 기반한AtchaSpeechCharacter조건부 렌더링은 맵이 로드되기 전 UI 문제를 방지하는 좋은 접근입니다.speechRequest패턴을 통한 새로운 캐릭터 메시지 시스템도 MVI 패턴에 부합합니다.
589-596: 콜백을 통한 상태 업데이트 패턴 적절함
isMapReadyCallback이 ViewModel의 상태를 업데이트하는 방식은 단방향 데이터 흐름(UDF) 패턴을 따르고 있습니다.
| versionCode = 21 | ||
| versionName = "1.3.2" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.kt" -o -name "*.java" | grep -i "viewmodel\|activity" | head -20Repository: depromeet/16th-team6-Android
Length of output: 1153
🏁 Script executed:
find . -type f \( -name "*HomeViewModel*" -o -name "*ViewModel.kt" \) | head -10Repository: depromeet/16th-team6-Android
Length of output: 919
🏁 Script executed:
rg "HomeViewModel|checkAppVersion" --type kt --type java -l | head -10Repository: depromeet/16th-team6-Android
Length of output: 98
🏁 Script executed:
rg "compareVersions" --type kt --type java -A 15 -B 2Repository: depromeet/16th-team6-Android
Length of output: 98
🏁 Script executed:
cat app/build.gradle.kts | grep -A 5 -B 5 "versionCode\|versionName"Repository: depromeet/16th-team6-Android
Length of output: 1194
🏁 Script executed:
git log --oneline -20 -- app/build.gradle.ktsRepository: depromeet/16th-team6-Android
Length of output: 235
🏁 Script executed:
git show HEAD~1:app/build.gradle.kts | grep -A 3 -B 3 "versionCode\|versionName" || echo "Could not retrieve previous version"Repository: depromeet/16th-team6-Android
Length of output: 161
🏁 Script executed:
cat app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeViewModel.ktRepository: depromeet/16th-team6-Android
Length of output: 31602
🏁 Script executed:
rg "compareVersions" -A 20 -B 2Repository: depromeet/16th-team6-Android
Length of output: 5215
versionName downgrade can break app update prompts
The version comparison in HomeViewModel.checkAppVersion() uses proper semantic versioning (major.minor.patch as integers). If versionName goes from "1.3.14" to "1.3.2", the numeric comparison will correctly identify patch 14 > 2, causing the app to report "UP_TO_DATE" when the server returns latest version "1.3.2". Users with 1.3.14 installed will not receive update notifications, even for critical patches in the new build.
Either confirm this version reset is intentional and adjust server version delivery accordingly, or keep versionName ≥ 1.3.14 to maintain update consistency.
🤖 Prompt for AI Agents
In app/build.gradle.kts around lines 42 to 43, the versionName was changed to
"1.3.2" while existing releases include "1.3.14", which will cause semantic
numeric comparisons in HomeViewModel.checkAppVersion() to treat 1.3.14 as newer
and suppress update prompts; either revert/increment versionName to be >= the
highest published version (e.g., set to "1.3.14" or higher) or coordinate with
the server to return a version string that is semantically greater than
installed builds; update the versionName appropriately and ensure versionCode is
increased to match the new versionName so update checks behave correctly.
| @@ -0,0 +1,213 @@ | |||
| package com.depromeet.team6.presentation.ui.common.speechbubble | |||
There was a problem hiding this comment.
파일 이름이 실제 용도와 불일치
이 파일은 AtchaSpeechBubbleTest.kt로 명명되어 있지만 테스트 파일이 아닌 프로덕션 Composable입니다. 이는 다른 개발자들에게 혼란을 줄 수 있고, 테스트 파일로 오인되어 빌드/패키징 시 의도치 않게 제외될 수 있습니다.
AtchaSpeechCharacter.kt 또는 SpeechBubbleStack.kt와 같은 명확한 이름으로 변경하세요.
🤖 Prompt for AI Agents
In app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble
AtchaSpeechBubbleTest.kt (line 1), the filename wrongly indicates a test file
although it contains production Composable code; rename the file to a clear
production name (e.g., AtchaSpeechCharacter.kt or SpeechBubbleStack.kt), update
any references/usages and imports in the codebase to the new filename, adjust
the Kotlin class/Composable name if it currently mismatches the new filename,
and run a full build/IDE sync to ensure no unresolved references remain.
| speechJob = scope.launch { | ||
| val removalJobs = mutableListOf<Job>() | ||
| // 말풍선 생성 로직 | ||
| messagesToAdd.forEach { messageText -> | ||
| // 부모 Job(speechJob)이 취소되면 delay에서 예외가 발생하며 중단됩니다. | ||
| delay(700L) | ||
| val newBubble = BubbleMessage( | ||
| id = System.nanoTime(), | ||
| text = messageText, | ||
| isVisible = true | ||
| ) | ||
| bubbles.add(newBubble) | ||
|
|
||
| // 개별 말풍선 제거 타이머 | ||
| val removalJob = scope.launch { | ||
| delay(2500L) | ||
| val idx = bubbles.indexOfFirst { it.id == newBubble.id } | ||
| if (idx != -1) { | ||
| bubbles[idx] = bubbles[idx].copy(isVisible = false) | ||
| } | ||
| delay(350) | ||
| val removeIdx = bubbles.indexOfFirst { it.id == newBubble.id } | ||
| if (removeIdx != -1) { | ||
| bubbles.removeAt(removeIdx) | ||
| } | ||
| } | ||
| removalJobs.add(removalJob) | ||
| } | ||
| // --- 모든 말풍선이 제거되면 speechJob 완료 -> 해당 라이프사이클 기준으로 쓰로틀링 --- | ||
| removalJobs.joinAll() | ||
| } |
There was a problem hiding this comment.
코루틴 Job 관리의 잠재적 메모리 누수 및 동작 불일치
removalJobs가 scope.launch로 생성되어 부모 speechJob이 취소되어도 개별 removal job들은 계속 실행됩니다. 이로 인해:
- 컴포저블이 dispose될 때 removal job이 계속 실행되어 메모리 누수 발생 가능
speechJob이 취소되어도bubbles상태가 계속 수정되어 예기치 않은 UI 동작 발생
scope.launch 대신 현재 코루틴 컨텍스트에서 launch하여 부모 Job 취소 시 함께 취소되도록 수정하세요:
speechJob = scope.launch {
val removalJobs = mutableListOf<Job>()
messagesToAdd.forEach { messageText ->
delay(700L)
val newBubble = BubbleMessage(
id = System.nanoTime(),
text = messageText,
isVisible = true
)
bubbles.add(newBubble)
- val removalJob = scope.launch {
+ val removalJob = launch {
delay(2500L)
val idx = bubbles.indexOfFirst { it.id == newBubble.id }
if (idx != -1) {
bubbles[idx] = bubbles[idx].copy(isVisible = false)
}
delay(350)
val removeIdx = bubbles.indexOfFirst { it.id == newBubble.id }
if (removeIdx != -1) {
bubbles.removeAt(removeIdx)
}
}
removalJobs.add(removalJob)
}
removalJobs.joinAll()
}📝 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.
| speechJob = scope.launch { | |
| val removalJobs = mutableListOf<Job>() | |
| // 말풍선 생성 로직 | |
| messagesToAdd.forEach { messageText -> | |
| // 부모 Job(speechJob)이 취소되면 delay에서 예외가 발생하며 중단됩니다. | |
| delay(700L) | |
| val newBubble = BubbleMessage( | |
| id = System.nanoTime(), | |
| text = messageText, | |
| isVisible = true | |
| ) | |
| bubbles.add(newBubble) | |
| // 개별 말풍선 제거 타이머 | |
| val removalJob = scope.launch { | |
| delay(2500L) | |
| val idx = bubbles.indexOfFirst { it.id == newBubble.id } | |
| if (idx != -1) { | |
| bubbles[idx] = bubbles[idx].copy(isVisible = false) | |
| } | |
| delay(350) | |
| val removeIdx = bubbles.indexOfFirst { it.id == newBubble.id } | |
| if (removeIdx != -1) { | |
| bubbles.removeAt(removeIdx) | |
| } | |
| } | |
| removalJobs.add(removalJob) | |
| } | |
| // --- 모든 말풍선이 제거되면 speechJob 완료 -> 해당 라이프사이클 기준으로 쓰로틀링 --- | |
| removalJobs.joinAll() | |
| } | |
| speechJob = scope.launch { | |
| val removalJobs = mutableListOf<Job>() | |
| // 말풍선 생성 로직 | |
| messagesToAdd.forEach { messageText -> | |
| // 부모 Job(speechJob)이 취소되면 delay에서 예외가 발생하며 중단됩니다. | |
| delay(700L) | |
| val newBubble = BubbleMessage( | |
| id = System.nanoTime(), | |
| text = messageText, | |
| isVisible = true | |
| ) | |
| bubbles.add(newBubble) | |
| // 개별 말풍선 제거 타이머 | |
| val removalJob = launch { | |
| delay(2500L) | |
| val idx = bubbles.indexOfFirst { it.id == newBubble.id } | |
| if (idx != -1) { | |
| bubbles[idx] = bubbles[idx].copy(isVisible = false) | |
| } | |
| delay(350) | |
| val removeIdx = bubbles.indexOfFirst { it.id == newBubble.id } | |
| if (removeIdx != -1) { | |
| bubbles.removeAt(removeIdx) | |
| } | |
| } | |
| removalJobs.add(removalJob) | |
| } | |
| // --- 모든 말풍선이 제거되면 speechJob 완료 -> 해당 라이프사이클 기준으로 쓰로틀링 --- | |
| removalJobs.joinAll() | |
| } |
| modifier = Modifier.noRippleClickable { | ||
| // 캐릭터 클릭시 애니메이션은 무조건 재생하되, API 호출만 throttling 처리 | ||
| if (speechJob?.isActive == false && !debouncing) { | ||
| debouncing = true | ||
| onCharacterClick() | ||
| scope.launch { | ||
| delay(1000) | ||
| debouncing = false | ||
| } | ||
| } | ||
| scope.launch { | ||
| // 매 클릭마다 0에서 1까지 1회 재생 | ||
| lottie.animate( | ||
| composition = composition, | ||
| iterations = 1, | ||
| initialProgress = 0f | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
클릭 핸들러 내 코루틴 로직 개선 필요
Line 166-173의 Lottie 애니메이션 실행은 debouncing 조건과 무관하게 항상 실행됩니다. 주석에 "애니메이션은 무조건 재생"이라고 명시되어 있어 의도된 동작으로 보이나, speechJob?.isActive == false 체크와 debouncing 체크가 분리되어 있어 의도 파악이 어렵습니다.
또한 Line 161-164의 debouncing 해제 코루틴은 예외 발생 시 debouncing = false가 실행되지 않아 영구적으로 클릭이 차단될 수 있습니다.
if (speechJob?.isActive == false && !debouncing) {
debouncing = true
onCharacterClick()
scope.launch {
- delay(1000)
- debouncing = false
+ try {
+ delay(1000)
+ } finally {
+ debouncing = false
+ }
}
}🤖 Prompt for AI Agents
In
app/src/main/java/com/depromeet/team6/presentation/ui/common/speechbubble/AtchaSpeechBubbleTest.kt
around lines 156-174, the click handler currently runs the Lottie animation
unconditionally while the API-throttling debouncing logic is separate, and the
coroutine that clears debouncing (lines 161-164) can fail to reset debouncing if
an exception occurs; refactor so the intent is explicit by either: 1) keeping
the animation launch outside the debouncing branch but moving the debouncing
reset into a try/finally inside the coroutine that sets debouncing (so
debouncing is always cleared even on error), or 2) running the animation inside
the same conditional branch as the API debouncing if you want them tied
together; implement the chosen approach and ensure any coroutine that sets
debouncing always clears it in a finally block and that animation coroutines are
launched on the same scope to avoid leaks.
| onTransportMarkerClick: (FocusedMarkerParameter) -> Unit = {}, | ||
| isMapReadyCallback: () -> Unit = {} | ||
| ) { |
There was a problem hiding this comment.
isMapReadyCallback 이 리컴포지션마다 호출되어 부작용이 중복 실행될 수 있습니다
현재 구현에서는:
var isMapReady by remember { mutableStateOf(false) }
// ...
tMapView.setOnMapReadyListener {
// ...
isMapReady = true
}
// ...
if (isMapReady) {
isMapReadyCallback()
} else {
AtChaLoadingView(...)
}isMapReady 가 한 번 true 가 된 이후로는, 해당 컴포저블이 리컴포즈될 때마다 isMapReadyCallback() 이 다시 호출됩니다. 이 콜백이:
- ForegroundService 시작
- ViewModel 이벤트 디스패치
- Analytics 이벤트 전송
등의 부작용을 포함할 경우, 동일 작업이 여러 번 실행되어 서비스 중복 시작, 불필요한 네트워크 호출, 상태 플리커 등이 발생할 수 있습니다.
의도가 “지도 준비 완료 시 1회만 콜백”이라면, 다음과 같이 one‑shot 패턴으로 바꾸는 것을 권장합니다:
- var isMapReady by remember { mutableStateOf(false) }
+ var isMapReady by remember { mutableStateOf(false) }
AndroidView(
// ...
factory = { context ->
val tMapView = TMapView(context)
// ...
tMapView.setOnMapReadyListener {
// 기존 로직 유지
// ...
isMapReady = true
}
tMapView
},
// ...
)
- if (isMapReady) {
- isMapReadyCallback()
- } else {
- AtChaLoadingView(
- transparent = false
- )
- }
+ LaunchedEffect(isMapReady) {
+ if (isMapReady) {
+ isMapReadyCallback()
+ }
+ }
+
+ if (!isMapReady) {
+ AtChaLoadingView(
+ transparent = false
+ )
+ }이렇게 하면:
isMapReady가false → true로 전이될 때 콜백이 정확히 한 번만 호출되고,- 이후 리컴포지션에서는 콜백이 재실행되지 않아 부작용이 중복되지 않습니다.
Also applies to: 337-343
| // AtchaSpeechCharacter( | ||
| // modifier = Modifier | ||
| // .offset(y = 10.dp), | ||
| // messagesToAdd = listOf( | ||
| // "테스트 테스트 야 야 야", | ||
| // "두번째 테스트 호롤로", | ||
| // "아라라라랄랄ㄹ라" | ||
| // ) | ||
| // ) | ||
|
|
||
| // if (hasAnyText) { | ||
| // Box { | ||
| // [email protected]( | ||
| // visible = isBottomSpeechBubbleVisible && showSpeechBubble, | ||
| // enter = fadeIn(), | ||
| // exit = fadeOut(), | ||
| // modifier = Modifier | ||
| // .offset(y = 10.dp) // 캐릭터 상단 기준 아래로 10dp 간격 | ||
| // ) { | ||
| // AtchaSpeechCharacter( | ||
| // messagesToAdd = listOf( | ||
| // "테스트 테스트 야 야 야", | ||
| // "두번째 테스트 호롤로", | ||
| // "아라라라랄랄ㄹ라" | ||
| // ) | ||
| // ) | ||
| // AtchaSpeechBubble( | ||
| // prefix = prefixText, | ||
| // modifier = Modifier, | ||
| // emphasisText = emphasisText, | ||
| // suffix = suffixText, | ||
| // tailExist = true | ||
| // ) | ||
| // } | ||
|
|
||
| // if (lineCount == 2) { | ||
| // androidx.compose.animation.AnimatedVisibility( | ||
| // visible = isTopSpeechBubbleVisible && showSpeechBubble, | ||
| // enter = fadeIn(), | ||
| // exit = fadeOut(), | ||
| // modifier = Modifier | ||
| // .align(Alignment.TopStart) | ||
| // .offset(y = (-26).dp) // 말풍선 높이 + 간격 | ||
| // ) { | ||
| // AtchaSpeechBubble( | ||
| // prefix = topPrefixText ?: "", | ||
| // modifier = Modifier, | ||
| // emphasisText = topEmphasisText, | ||
| // suffix = topSuffixText, | ||
| // tailExist = false | ||
| // ) | ||
| // } | ||
| // } | ||
| // } | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
대량의 주석 처리된 코드 제거 필요
약 55줄의 주석 처리된 코드가 프로덕션 파일에 남아있습니다. 이는 코드 가독성을 심각하게 저해하고 향후 유지보수를 어렵게 만듭니다.
Git 히스토리를 통해 이전 구현을 확인할 수 있으므로, 주석 처리된 코드는 완전히 제거하세요. 추후 참조가 필요하다면 별도 브랜치나 태그로 보존하는 것이 좋습니다.
🤖 Prompt for AI Agents
In
app/src/main/java/com/depromeet/team6/presentation/ui/home/component/CharacterLottieSpeechBubble.kt
around lines 121 to 175, there are ~55 lines of commented-out code left in the
production file; remove the entire commented block (all commented
AtchaSpeechCharacter/AtchaSpeechBubble attempts and related AnimatedVisibility
scaffolding) so the file contains only active code, then commit the change; if
you want to preserve the old implementation for reference, create a branch or
tag or rely on git history rather than leaving commented code in the file.
| mapModified: () -> Unit, | ||
| isMapReadyCallback: () -> Unit | ||
| ) { |
There was a problem hiding this comment.
TMapViewCompose 의 isMapReadyCallback 도 리컴포지션마다 반복 호출됩니다
여기서도 isMapReady 가 true 로 한 번 바뀐 이후에는, 컴포저블가 리컴포즈될 때마다 콜백이 다시 실행됩니다:
var isMapReady by remember { mutableStateOf(false) }
// ...
tMapView.setOnMapReadyListener {
// ...
isMapReady = true
}
// ...
if (isMapReady) {
isMapReadyCallback()
Image( /* 출발 마커 렌더링 */ )
} else {
AtChaLoadingView(...)
}이 구조에서는:
- 콜백이 ViewModel 이벤트 디스패치, ForegroundService 시작, 분석 이벤트 전송 등에 연결되어 있을 경우,
- 스크롤/상태 변경으로 인한 리컴포지션마다 동일 부작용이 반복 실행될 수 있습니다.
AfterRegisterMap 과 마찬가지로, “지도 준비 완료 시 한 번만 알려주는” 용도라면 LaunchedEffect 를 통해 one‑shot 으로 바꾸는 편이 안전합니다:
- var isMapReady by remember { mutableStateOf(false) }
+ var isMapReady by remember { mutableStateOf(false) }
// ...
AndroidView(
// ...
factory = { context ->
val tMapView = TMapView(context)
// ...
tMapView.setOnMapReadyListener {
// 기존 초기화 로직 유지
// ...
isMapReady = true
}
tMapView
},
// ...
)
- if (isMapReady) {
- isMapReadyCallback()
- // 출발 마커
- Image(...)
- } else {
- AtChaLoadingView(transparent = false)
- }
+ LaunchedEffect(isMapReady) {
+ if (isMapReady) {
+ isMapReadyCallback()
+ }
+ }
+
+ if (isMapReady) {
+ // 출발 마커
+ Image(...)
+ } else {
+ AtChaLoadingView(transparent = false)
+ }이렇게 하면:
- 콜백은
false → true전이 시점에 한 번만 호출되고, - 이후 지도 상호작용/리컴포지션에도 중복 부작용이 발생하지 않습니다.
Also applies to: 165-175
| // val characterTexts = CharacterTexts( | ||
| // taxiCostText = stringResource(R.string.home_bubble_basic_text), | ||
| // aboutText = stringResource(R.string.home_bubble_least_text), | ||
| // wonText = stringResource(R.string.home_bubble_won_text), | ||
| // moveMapText = stringResource(R.string.home_bubble_map_text), | ||
| // | ||
| // expectDepartText = stringResource(R.string.home_bubble_alarm_emphasis_text), | ||
| // expectTaxiCostText = stringResource(R.string.home_bubble_user_departure_taxi_cost_text), | ||
| // expectBustDepartureText = stringResource(R.string.home_bubble_before_bus_departure_text), | ||
| // | ||
| // expectTimeClickedText1 = stringResource(R.string.home_bubble_expect_departure_time_clicked_text_1), | ||
| // expectTimeClickedText2 = stringResource(R.string.home_bubble_expect_departure_time_clicked_text_2), | ||
| // changeLocationClickedText = stringResource(R.string.home_bubble_location_clicked_text), | ||
| // | ||
| // busDepartureText1 = stringResource(R.string.home_bubble_bus_departure_text_1), | ||
| // busDepartureText2 = stringResource(R.string.home_bubble_bus_departure_text_2), | ||
| // subwayDepartureText1 = stringResource(R.string.home_bubble_subway_departure_text_1), | ||
| // subwayDepartureText2 = stringResource(R.string.home_bubble_subway_departure_text_2), | ||
| // timeInfoText1 = stringResource(R.string.home_bubble_time_info_text_1), | ||
| // timeInfoText2 = stringResource(R.string.home_bubble_time_info_text_2), | ||
| // departureTimeText1 = stringResource(R.string.home_bubble_departure_time_info_text_1), | ||
| // busDepartureTaxiCostText = stringResource(R.string.home_bubble_departed_taxi_cost_text), | ||
| // trustText1 = stringResource(R.string.home_bubble_trust_text_1), | ||
| // trustText2 = stringResource(R.string.home_bubble_trust_text_2), | ||
| // | ||
| // userDepartureBusText = stringResource(R.string.home_bubble_user_departure_bus_text), | ||
| // userDepartureSubwayText = stringResource(R.string.home_bubble_user_departure_subway_text), | ||
| // userDepartureDownText = stringResource(R.string.home_bubble_user_departure_down_text), | ||
| // userDepartureDetailBtnText = stringResource(R.string.home_bubble_user_departure_detail_btn_text), | ||
| // userDepartureCheckDetailText = stringResource(R.string.home_bubble_user_departure_check_detail_text) | ||
| // ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
대규모 주석 처리된 코드 제거 권장
파일 전반에 걸쳐 150줄 이상의 주석 처리된 코드가 존재합니다 (CharacterTexts, baseCharacterData, showTempMessage 함수 등). 새로운 speech 시스템으로의 전환이 완료되면 이 코드들을 정리해야 합니다. Git 히스토리에서 필요시 복구할 수 있으므로 주석 상태로 유지할 필요가 없습니다.
🤖 Prompt for AI Agents
In app/src/main/java/com/depromeet/team6/presentation/ui/home/HomeScreen.kt
around lines 112-142 there is a large commented-out CharacterTexts block (and
the review notes other big commented blocks like baseCharacterData and
showTempMessage) that should be removed: delete these commented code blocks from
the file (and any other similar large commented sections in this file) ensuring
you do not remove any live references, run a build/tests to confirm nothing
depended on them, and commit the cleanup with a message like "chore(home):
remove obsolete commented speech-system code (recoverable via git history)".
| - **빌드 시스템**: Gradle (Kotlin DSL - `build.gradle.kts`) | ||
| - **CI/CD**: GitHub Actions (`.github/workflows/android_ci.yml`)를 통해 CI 파이프라인이 구축되어 있어 코드 통합 및 빌드 자동화를 수행합니다. | ||
| - **코드 품질**: Ktlint 플러그인을 사용하여 코드 스타일을 일관되게 유지합니다. | ||
| - **코드 품질**: Kt-lint 플러그인을 사용하여 코드 스타일을 일관되게 유지합니다. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
공식 명칭과 불일치
Ktlint의 공식 명칭은 하이픈 없이 "Ktlint"입니다. 문서 일관성을 위해 원래 표기를 유지하는 것이 좋습니다.
-- **코드 품질**: Kt-lint 플러그인을 사용하여 코드 스타일을 일관되게 유지합니다.
+- **코드 품질**: Ktlint 플러그인을 사용하여 코드 스타일을 일관되게 유지합니다.📝 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.
| - **코드 품질**: Kt-lint 플러그인을 사용하여 코드 스타일을 일관되게 유지합니다. | |
| - **코드 품질**: Ktlint 플러그인을 사용하여 코드 스타일을 일관되게 유지합니다. |
🤖 Prompt for AI Agents
In GEMINI.md around line 57, the document uses the incorrect official name
"Kt-lint"; replace that instance with the correct official spelling "Ktlint" to
maintain consistency in the docs (update the line to read: **코드 품질**: Ktlint
플러그인을 사용하여 코드 스타일을 일관되게 유지합니다.).
- 브로드캐스트 발송 로직을 명시적 브로드캐스트로 수정했습니다.
2 participants
#️⃣연관된 이슈
📝작업 내용
PR 발행 전 체크 리스트
스크린샷 (선택)
💬리뷰 요구사항(선택)
Summary by CodeRabbit
새로운 기능
리팩터
문서/리소스
테스트
잡일
✏️ Tip: You can customize this high-level summary in your review settings.