Conversation
- feature/alarm 모듈 신규 생성 (build.gradle.kts, AndroidManifest, strings) - AlarmRing, AlarmSetting 화면을 feature/home → feature/alarm으로 이동 - feature/home에서 AlarmForegroundService 및 alarm 관련 리소스 제거 - settings.gradle.kts, libs.versions.toml, app/build.gradle.kts 의존성 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- AlarmInfo, AlarmRepository에 위치 추적 관련 필드/메서드 추가 - LocationRepository, LocationRepositoryImpl 신규 구현 (FusedLocationProvider) - NearestNodeResult, Coordinate 모델 추가 - ObserveNearestNodeUseCase, GetNodesByRouteUseCase, ObserveAlarmTriggerUseCase 추가 - AlarmDataSource에 nearestNode StateFlow 및 triggerAlarm 로직 추가 - DistanceUtil (Haversine 거리 계산) 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- AlarmMonitor 화면 추가 (노선 노드 목록, 현재 정류장 표시, 잔여 정류장 수) - AlarmMonitorViewModel: 노드 로드, 위치 추적(observeNearestNode), AlarmRing 트리거 감지 - AlarmForegroundService: 위치 추적 포그라운드 서비스, API 실패 시 5초 재시도 - AlarmNavigation: 서비스 시작/종료 람다를 navigation 레이어에서 처리 - Route.kt에 AlarmMonitorRoute 추가 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- HomeViewModel: 알람 상태 감지 및 AlarmRing/AlarmMonitor 진입 처리 - HomeScreen, HomeContract: 알람 배너 및 알람 설정 진입 UI 추가 - BoardingBanner: 진행 중 알람 표시 컴포넌트 - HomeNavigation: AlarmSetting, AlarmMonitor, AlarmRing 네비게이션 콜백 연결 - BusNode, BusRoute 화면: 알람 설정 진입 지점 추가 - AppNavHost: homeGraph/alarmGraph 통합, 서비스 람다 위임 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Open
2 tasks
Walkthrough
Changes위치 기반 알람 파이프라인 및 feature:alarm 모듈
Sequence Diagram(s)sequenceDiagram
actor 사용자
participant AlarmSettingScreen
participant AlarmForegroundService
participant LocationRepositoryImpl
participant AlarmDataSource
participant AlarmMonitorScreen
participant AlarmRingScreen
사용자->>AlarmSettingScreen: 알람 설정 확인 (위치/알림 권한 획득 후)
AlarmSettingScreen->>AlarmForegroundService: startAlarmService(routeNo, nodeName)
AlarmForegroundService->>LocationRepositoryImpl: observeNearestNode(nodes, boardingIndex, destIndex)
loop 위치 업데이트마다
LocationRepositoryImpl-->>AlarmForegroundService: NearestNodeResult
AlarmForegroundService->>AlarmDataSource: updateNearestNode(result)
AlarmMonitorScreen-->>사용자: 남은 정류장 수 갱신
end
AlarmForegroundService->>AlarmDataSource: triggerAlarm() (remaining 조건 충족)
AlarmDataSource-->>AlarmMonitorScreen: isTriggered = true
AlarmMonitorScreen->>AlarmRingScreen: navigateToAlarmRing (launchSingleTop)
사용자->>AlarmRingScreen: 알람 해제 확인
AlarmRingScreen->>AlarmForegroundService: stopAlarmService()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
core/common/src/main/java/com/choiminjun/common/util/DistanceUtil.kt (1)
12-25: 🧹 Nitpick | 🔵 Trivial | 💤 Low value좌표 범위 검증 추가를 고려하세요.
현재 구현은 유효하지 않은 좌표(위도 범위: -90
90, 경도 범위: -180180 벗어남)에 대한 검증이 없어, 잘못된 입력 시 의미 없는 거리 값을 반환할 수 있습니다. GPS 센서나 API로부터 받은 좌표가 항상 유효하다고 가정하는 경우 문제없지만, 방어적 프로그래밍 차원에서 검증 로직 추가를 권장합니다.🛡️ 입력 검증 추가 예시
fun calculateDistance( lat1: Double, lon1: Double, lat2: Double, lon2: Double, ): Double { + require(lat1 in -90.0..90.0) { "lat1 must be in range -90..90" } + require(lat2 in -90.0..90.0) { "lat2 must be in range -90..90" } + require(lon1 in -180.0..180.0) { "lon1 must be in range -180..180" } + require(lon2 in -180.0..180.0) { "lon2 must be in range -180..180" } + val phi1 = Math.toRadians(lat1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/common/src/main/java/com/choiminjun/common/util/DistanceUtil.kt` around lines 12 - 25, The calculateDistance function does not validate that input coordinates fall within valid GPS ranges (latitude between -90 and 90, longitude between -180 and 180), which can result in meaningless distance calculations when invalid coordinates are passed. Add input validation logic at the beginning of the calculateDistance function to check that lat1, lat2 are within -90 to 90 and lon1, lon2 are within -180 to 180, throwing an IllegalArgumentException with a descriptive message if any coordinate is out of range to enforce defensive programming practices.core/domain/src/main/java/com/choiminjun/domain/repository/LocationRepository.kt (1)
10-14: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win파라미터 제약조건 문서화를 고려하세요.
observeNearestNode메서드가boardingIndex와destIndex에 대해 유효한 범위(0 <= boardingIndex < destIndex < nodes.size)를 가정하지만, 인터페이스 레벨에서 이 제약조건이 명시되지 않았습니다. KDoc 주석으로 precondition을 문서화하면 구현체와 호출자 간 계약이 더 명확해집니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/domain/src/main/java/com/choiminjun/domain/repository/LocationRepository.kt` around lines 10 - 14, The observeNearestNode method lacks KDoc documentation for its parameter constraints. Add KDoc comments above the observeNearestNode function signature to document the preconditions for the parameters, specifically that boardingIndex and destIndex must satisfy the range constraint 0 <= boardingIndex < destIndex < nodes.size. This will clarify the contract between the interface and its implementations for both callers and developers maintaining the code.app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt (1)
102-107: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win재시도 간격을 상수로 추출하여 테스트 및 유지보수성을 개선하세요.
재시도 지연 시간
5.seconds가 하드코딩되어 있습니다. 이를 companion object 상수로 추출하면 테스트에서 오버라이드하거나 향후 조정하기 쉬워집니다.♻️ 제안: 상수로 추출
+ companion object { + private const val NOTIFICATION_ID = 1001 + private const val CHANNEL_ID = "stop_alarm_channel" + private val LOCATION_RETRY_DELAY = 5.seconds + const val EXTRA_ROUTE_NO = "extra_route_no" + const val EXTRA_NODE_NAME = "extra_node_name" + } private fun startLocationTracking(alarmInfo: AlarmInfo) { ... } catch (e: Exception) { Timber.e(e, "위치 추적 오류, 5초 후 재시도") - delay(5.seconds) + delay(LOCATION_RETRY_DELAY) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt` around lines 102 - 107, Extract the hardcoded retry delay value of 5.seconds from the catch block in the exception handler into a companion object constant. Define a constant (e.g., LOCATION_TRACKING_RETRY_DELAY) in the companion object of the AlarmForegroundService class and replace the delay(5.seconds) call with delay(LOCATION_TRACKING_RETRY_DELAY) to improve testability and make future adjustments easier.feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorScreen.kt (1)
199-199: 🧹 Nitpick | 🔵 Trivial | 💤 Low value문자열 비교로 현재 정류장을 매칭할 때 공백 차이에 취약할 수 있습니다.
nodeName == nearestNodeName비교는 대소문자와 공백을 정확히 일치시켜야 합니다. 데이터 소스에서 불일치가 있으면 현재 위치 표시가 실패할 수 있습니다.데이터 일관성이 보장된다면 현재 구현도 충분하지만, 향후 안정성을 위해 trim() 처리를 고려해볼 수 있습니다.
💡 선택적 개선안
- isCurrentNode = nodeName == nearestNodeName, + isCurrentNode = nodeName.trim() == nearestNodeName?.trim(),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorScreen.kt` at line 199, The string comparison in the isCurrentNode assignment uses nodeName == nearestNodeName which is vulnerable to whitespace differences that could cause the current station matching to fail. Apply trim() to both nodeName and nearestNodeName strings before comparing them to handle potential leading or trailing whitespace that may exist in the data source. This ensures robust matching even if the data contains unexpected whitespace variations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt`:
- Around line 89-110: The `while (isActive)` loop in the startLocationTracking
method causes infinite resubscription when observeNearestNode().collect()
completes normally. Instead of looping indefinitely, the flow should only retry
on exceptions, not on normal completion. Modify the logic so that when the Flow
completes without an exception, the loop breaks and stops the resubscription,
while still implementing the 5-second retry delay only for actual Exception
cases caught in the catch block. This way, normal completion (when the
repository intentionally closes the Flow) will properly terminate location
tracking instead of immediately restarting the subscription.
- Around line 97-100: The issue is that stopSelf() asynchronously terminates the
service while the Flow collection continues, potentially causing multiple
triggerAlarm() calls. To fix this, cancel the Job that is managing the Flow
collection immediately before calling stopSelf(). This ensures the collect block
stops executing and prevents duplicate alarm triggers. Locate where the Job is
created for the Flow collection (likely in a lifecycleScope or separate job
variable) and call cancel() on it before the stopSelf() call in the conditional
block where result.remaining is in the range 0..alarmInfo.stopsBeforeAlarm.
- Around line 58-61: The code in the scope.launch block uses
observeAlarm().first() which captures the initial emission with a blank routeId
before any alarm is configured, causing the if condition checking
alarm.routeId.isNotBlank() to immediately fail and never start location
tracking. Modify the observeAlarm() chain to filter for alarms with non-blank
routeIds using a filter operation, so that the first() call waits for a valid
alarm with a populated routeId rather than immediately capturing the initial
blank value. This ensures startLocationTracking(alarm) is called with a properly
configured alarm instead of being skipped due to the blank default value.
In
`@core/data/src/main/java/com/choiminjun/data/repository/LocationRepositoryImpl.kt`:
- Around line 61-75: The code has two critical issues: (1) The nodes.subList
call at line 61 lacks validation for boardingIndex and destIndex parameters,
which can cause IndexOutOfBoundsException if they are negative, out of range, or
boardingIndex exceeds destIndex. Add bounds checking before the subList call to
ensure both indices are within valid range and boardingIndex is less than or
equal to destIndex. (2) The !! operators used on windowNodes[i].latitude and
windowNodes[i].longitude assume these nullable fields are never null, but they
can be, causing NPE. Replace the non-null assertion operators with safe null
handling such as using filterNotNull, safe let expressions, or proper null
checks to handle cases where coordinates might be null and skip processing those
nodes accordingly.
- Around line 34-54: The observeLocation() method in LocationRepositoryImpl
suppresses the MissingPermission lint warning but lacks runtime permission
validation, creating a defensive coding gap where future callers could trigger
SecurityException. Either add runtime permission validation using
ActivityCompat.checkSelfPermission() to verify
android.permission.ACCESS_FINE_LOCATION and
android.permission.ACCESS_COARSE_LOCATION before calling
fusedClient.requestLocationUpdates(), or add comprehensive KDoc documentation to
the observeLocation() method and its interface declaration explicitly stating
that the caller must ensure required location permissions (ACCESS_FINE_LOCATION
and ACCESS_COARSE_LOCATION) are granted before invoking this method.
In
`@core/datastore/src/main/java/com/choiminjun/datastore/source/AlarmDataSource.kt`:
- Around line 62-66: The isTriggered flag in the triggerAlarm() function is
persisted in DataStore but not properly reset when the app restarts, causing
unintended AlarmRing navigation if the app crashes between triggering and user
dismissal. Add initialization logic to clear or reset the KEY_IS_TRIGGERED flag
on app startup (in the StopReminderApplication initialization or equivalent
startup location) and ensure the flag is also reset in the setAlarm() function
to restore the normal state. Alternatively, consider making this flag transient
so it does not persist across app restarts, or implement a recovery mechanism
that resets it when the alarm service is reinitialized.
In
`@core/domain/src/main/java/com/choiminjun/domain/usecase/ObserveAlarmTriggerUseCase.kt`:
- Line 15: The filter expression in ObserveAlarmTriggerUseCase uses a range
check with `alarmInfo.stopsBeforeAlarm` which could behave unexpectedly if
negative values are passed. Add validation in AlarmSettingViewModel to ensure
`selectedStopsBefore` is always validated as a positive number (greater than or
equal to 1) before it is used in the SelectStopsBefore intent. This prevents
negative or out-of-range values from reaching the filter logic at line 15 and
ensures the range condition works as intended.
In
`@core/domain/src/main/java/com/choiminjun/domain/usecase/ObserveNearestNodeUseCase.kt`:
- Line 21: The condition checking for invalid indices (boardingIndex == -1,
destIndex == -1, or boardingIndex >= destIndex) in ObserveNearestNodeUseCase
silently returns from the flow without logging. Add a warning log statement
before the return@flow that includes the actual values of boardingIndex and
destIndex to help with debugging when these invalid conditions occur. This will
provide visibility into why the flow terminated without changing the existing
control flow logic.
In
`@feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorScreen.kt`:
- Line 188: The itemsIndexed function in AlarmMonitorScreen.kt uses nodeName as
the key parameter, but since bus routes can have duplicate station names, this
creates a key collision risk during Compose recomposition that may cause UI
errors or runtime exceptions. Replace the key lambda in the itemsIndexed call to
use a combination that ensures uniqueness, such as combining the index with the
nodeName (for example "index-nodeName") or simply using the index value alone if
the display order is stable and unique.
In
`@feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorViewModel.kt`:
- Around line 35-42: The observeAlarm() method in AlarmMonitorViewModel has
asymmetric state update logic where the if block (when isTriggered=true) only
posts a side effect without updating state, while the else block (when
isTriggered=false) updates state via reduce. To fix this, add a state update
using reduce in the if block where AlarmMonitorSideEffect.AlarmTriggered is
posted, so that the state is consistently updated along with the side effect.
This ensures that when the observer re-emits after clearAlarm() is called, the
state changes are properly captured without delayed updates after navigation.
In
`@feature/alarm/src/main/java/com/choiminjun/alarm/navigation/AlarmNavigation.kt`:
- Around line 28-37: The onBackClick callback in the AlarmMonitorRoute
composable only calls navigateBack without stopping the alarm service, causing
the service to continue running in the background. Modify the onBackClick lambda
to call stopAlarmService() before navigateBack, similar to how it is done in the
onAlarmStopped callback, ensuring the service is properly stopped when the user
navigates away from the AlarmMonitor screen.
---
Nitpick comments:
In
`@app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt`:
- Around line 102-107: Extract the hardcoded retry delay value of 5.seconds from
the catch block in the exception handler into a companion object constant.
Define a constant (e.g., LOCATION_TRACKING_RETRY_DELAY) in the companion object
of the AlarmForegroundService class and replace the delay(5.seconds) call with
delay(LOCATION_TRACKING_RETRY_DELAY) to improve testability and make future
adjustments easier.
In `@core/common/src/main/java/com/choiminjun/common/util/DistanceUtil.kt`:
- Around line 12-25: The calculateDistance function does not validate that input
coordinates fall within valid GPS ranges (latitude between -90 and 90, longitude
between -180 and 180), which can result in meaningless distance calculations
when invalid coordinates are passed. Add input validation logic at the beginning
of the calculateDistance function to check that lat1, lat2 are within -90 to 90
and lon1, lon2 are within -180 to 180, throwing an IllegalArgumentException with
a descriptive message if any coordinate is out of range to enforce defensive
programming practices.
In
`@core/domain/src/main/java/com/choiminjun/domain/repository/LocationRepository.kt`:
- Around line 10-14: The observeNearestNode method lacks KDoc documentation for
its parameter constraints. Add KDoc comments above the observeNearestNode
function signature to document the preconditions for the parameters,
specifically that boardingIndex and destIndex must satisfy the range constraint
0 <= boardingIndex < destIndex < nodes.size. This will clarify the contract
between the interface and its implementations for both callers and developers
maintaining the code.
In
`@feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorScreen.kt`:
- Line 199: The string comparison in the isCurrentNode assignment uses nodeName
== nearestNodeName which is vulnerable to whitespace differences that could
cause the current station matching to fail. Apply trim() to both nodeName and
nearestNodeName strings before comparing them to handle potential leading or
trailing whitespace that may exist in the data source. This ensures robust
matching even if the data contains unexpected whitespace variations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89f095fc-1710-46bf-b560-a6eb4547b110
📒 Files selected for processing (51)
app/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/com/choiminjun/stopreminder/StopReminderApplication.ktapp/src/main/java/com/choiminjun/stopreminder/navigation/AppNavHost.ktapp/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.ktbuild-logic/src/main/kotlin/com/choiminjun/build/logic/KotlinAndroid.ktcore/common/src/main/java/com/choiminjun/common/util/DistanceUtil.ktcore/data/build.gradle.ktscore/data/src/main/java/com/choiminjun/data/di/DataModule.ktcore/data/src/main/java/com/choiminjun/data/repository/AlarmRepositoryImpl.ktcore/data/src/main/java/com/choiminjun/data/repository/LocationRepositoryImpl.ktcore/datastore/src/main/java/com/choiminjun/datastore/source/AlarmDataSource.ktcore/domain/build.gradle.ktscore/domain/src/main/java/com/choiminjun/domain/model/alarm/AlarmInfo.ktcore/domain/src/main/java/com/choiminjun/domain/model/location/Coordinate.ktcore/domain/src/main/java/com/choiminjun/domain/model/location/NearestNodeResult.ktcore/domain/src/main/java/com/choiminjun/domain/repository/AlarmRepository.ktcore/domain/src/main/java/com/choiminjun/domain/repository/LocationRepository.ktcore/domain/src/main/java/com/choiminjun/domain/usecase/GetNodesByRouteUseCase.ktcore/domain/src/main/java/com/choiminjun/domain/usecase/ObserveAlarmTriggerUseCase.ktcore/domain/src/main/java/com/choiminjun/domain/usecase/ObserveNearestNodeUseCase.ktcore/navigation/src/main/java/com/choiminjun/navigation/Route.ktfeature/alarm/.gitignorefeature/alarm/build.gradle.ktsfeature/alarm/src/main/AndroidManifest.xmlfeature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorContract.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorScreen.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorViewModel.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmring/AlarmRingContract.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmring/AlarmRingScreen.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmring/AlarmRingViewModel.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingContract.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingScreen.ktfeature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingViewModel.ktfeature/alarm/src/main/java/com/choiminjun/alarm/navigation/AlarmNavigation.ktfeature/alarm/src/main/res/values/strings.xmlfeature/home/src/main/AndroidManifest.xmlfeature/home/src/main/java/com/choiminjun/home/home/HomeContract.ktfeature/home/src/main/java/com/choiminjun/home/home/HomeScreen.ktfeature/home/src/main/java/com/choiminjun/home/home/HomeViewModel.ktfeature/home/src/main/java/com/choiminjun/home/home/component/BoardingBanner.ktfeature/home/src/main/java/com/choiminjun/home/navigation/HomeNavigation.ktfeature/home/src/main/java/com/choiminjun/home/node/BusNodeContract.ktfeature/home/src/main/java/com/choiminjun/home/node/BusNodeScreen.ktfeature/home/src/main/java/com/choiminjun/home/node/BusNodeViewModel.ktfeature/home/src/main/java/com/choiminjun/home/route/BusRouteContract.ktfeature/home/src/main/java/com/choiminjun/home/route/BusRouteScreen.ktfeature/home/src/main/java/com/choiminjun/home/route/BusRouteViewModel.ktfeature/home/src/main/res/values/strings.xmlgradle/libs.versions.tomlsettings.gradle.kts
💤 Files with no reviewable changes (2)
- feature/home/src/main/res/values/strings.xml
- feature/home/src/main/AndroidManifest.xml
Comment on lines
+58
to
+61
| scope.launch { | ||
| val alarm = alarmRepository.observeAlarm().first() | ||
| if (alarm.routeId.isNotBlank()) startLocationTracking(alarm) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: AlarmDataSource의 AlarmInfo 초기값 확인
rg -nP -A5 'AlarmInfo\(' --type=kt --glob '*AlarmDataSource*'Repository: mj010504/StopReminder
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Find AlarmDataSource file
fd -i 'AlarmDataSource' --type f
# Find AlarmInfo data class
rg -n 'data class AlarmInfo' --type kotlinRepository: mj010504/StopReminder
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Search for AlarmInfo instantiation patterns
rg -n 'AlarmInfo(' -A 3 --type kotlin --max-count=5Repository: mj010504/StopReminder
Length of output: 136
🏁 Script executed:
#!/bin/bash
# Check AlarmForegroundService implementation
fd -i 'AlarmForegroundService' --type f -exec wc -l {} \;Repository: mj010504/StopReminder
Length of output: 252
🏁 Script executed:
#!/bin/bash
# Read AlarmInfo definition
cat -n core/domain/src/main/java/com/choiminjun/domain/model/alarm/AlarmInfo.ktRepository: mj010504/StopReminder
Length of output: 493
🏁 Script executed:
#!/bin/bash
# Read AlarmDataSource
cat -n core/datastore/src/main/java/com/choiminjun/datastore/source/AlarmDataSource.ktRepository: mj010504/StopReminder
Length of output: 4000
🏁 Script executed:
#!/bin/bash
# Read AlarmForegroundService around lines 58-61
cat -n app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt | head -120 | tail -80Repository: mj010504/StopReminder
Length of output: 3477
AlarmDataSource에서 routeId가 빈 문자열로 초기화되므로 서비스 시작 직후 즉시 종료됩니다.
AlarmInfo의 routeId는 기본값이 빈 문자열 ""이며, AlarmDataSource도 저장된 값이 없으면 빈 문자열을 반환합니다. 따라서 서비스 시작 시 observeAlarmState()는 첫 번째 emission에서 이미 routeId.isBlank()를 감지하고 즉시 stopSelf()를 호출합니다. 동시에 라인 60의 if (alarm.routeId.isNotBlank())는 실패하여 startLocationTracking()이 시작되지 않습니다.
알람이 설정되기 전에 서비스가 시작될 경우 위치 추적이 전혀 시작되지 않고 서비스가 바로 종료되는 문제가 발생합니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt`
around lines 58 - 61, The code in the scope.launch block uses
observeAlarm().first() which captures the initial emission with a blank routeId
before any alarm is configured, causing the if condition checking
alarm.routeId.isNotBlank() to immediately fail and never start location
tracking. Modify the observeAlarm() chain to filter for alarms with non-blank
routeIds using a filter operation, so that the first() call waits for a valid
alarm with a populated routeId rather than immediately capturing the initial
blank value. This ensures startLocationTracking(alarm) is called with a properly
configured alarm instead of being skipped due to the blank default value.
Comment on lines
+89
to
110
| private fun startLocationTracking(alarmInfo: AlarmInfo) { | ||
| locationJob?.cancel() | ||
| locationJob = scope.launch { | ||
| while (isActive) { | ||
| try { | ||
| observeNearestNode(alarmInfo.routeId, alarmInfo.boardingNodeId, alarmInfo.destNodeId) | ||
| .collect { result -> | ||
| alarmRepository.updateNearestNode(result) | ||
| if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) { | ||
| alarmRepository.triggerAlarm() | ||
| stopSelf() | ||
| } | ||
| } | ||
| } catch (e: CancellationException) { | ||
| throw e | ||
| } catch (e: Exception) { | ||
| Timber.e(e, "위치 추적 오류, 5초 후 재시도") | ||
| delay(5.seconds) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
while (isActive) 무한 루프는 정상 종료 시에도 계속 재시도합니다.
observeNearestNode().collect()가 정상 완료되면 루프는 즉시 다시 구독을 시작합니다. Flow가 upstream에서 완료되는 경우(예: 저장소가 특정 조건에서 Flow를 닫는 경우) 무한 재구독이 발생할 수 있습니다.
♻️ 제안: 정상 완료와 오류를 구분하는 로직 추가
private fun startLocationTracking(alarmInfo: AlarmInfo) {
locationJob?.cancel()
locationJob = scope.launch {
- while (isActive) {
- try {
- observeNearestNode(alarmInfo.routeId, alarmInfo.boardingNodeId, alarmInfo.destNodeId)
- .collect { result ->
- alarmRepository.updateNearestNode(result)
- if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) {
- alarmRepository.triggerAlarm()
- stopSelf()
- }
- }
- } catch (e: CancellationException) {
- throw e
- } catch (e: Exception) {
- Timber.e(e, "위치 추적 오류, 5초 후 재시도")
- delay(5.seconds)
- }
+ try {
+ observeNearestNode(alarmInfo.routeId, alarmInfo.boardingNodeId, alarmInfo.destNodeId)
+ .collect { result ->
+ alarmRepository.updateNearestNode(result)
+ if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) {
+ alarmRepository.triggerAlarm()
+ stopSelf()
+ }
+ }
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
+ Timber.e(e, "위치 추적 오류")
+ stopSelf()
}
}
}또는 ObserveNearestNodeUseCase 내부에서 무한 재시도를 처리하도록 설계되었다면, PR 목표에서 언급한 "5초 재시도"가 서비스 레벨이 아닌 UseCase/Repository 레벨에서 처리되는지 확인하세요.
📝 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.
Suggested change
| private fun startLocationTracking(alarmInfo: AlarmInfo) { | |
| locationJob?.cancel() | |
| locationJob = scope.launch { | |
| while (isActive) { | |
| try { | |
| observeNearestNode(alarmInfo.routeId, alarmInfo.boardingNodeId, alarmInfo.destNodeId) | |
| .collect { result -> | |
| alarmRepository.updateNearestNode(result) | |
| if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) { | |
| alarmRepository.triggerAlarm() | |
| stopSelf() | |
| } | |
| } | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| Timber.e(e, "위치 추적 오류, 5초 후 재시도") | |
| delay(5.seconds) | |
| } | |
| } | |
| } | |
| } | |
| private fun startLocationTracking(alarmInfo: AlarmInfo) { | |
| locationJob?.cancel() | |
| locationJob = scope.launch { | |
| try { | |
| observeNearestNode(alarmInfo.routeId, alarmInfo.boardingNodeId, alarmInfo.destNodeId) | |
| .collect { result -> | |
| alarmRepository.updateNearestNode(result) | |
| if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) { | |
| alarmRepository.triggerAlarm() | |
| stopSelf() | |
| } | |
| } | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| Timber.e(e, "위치 추적 오류") | |
| stopSelf() | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt`
around lines 89 - 110, The `while (isActive)` loop in the startLocationTracking
method causes infinite resubscription when observeNearestNode().collect()
completes normally. Instead of looping indefinitely, the flow should only retry
on exceptions, not on normal completion. Modify the logic so that when the Flow
completes without an exception, the loop breaks and stops the resubscription,
while still implementing the 5-second retry delay only for actual Exception
cases caught in the catch block. This way, normal completion (when the
repository intentionally closes the Flow) will properly terminate location
tracking instead of immediately restarting the subscription.
Comment on lines
+97
to
+100
| if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) { | ||
| alarmRepository.triggerAlarm() | ||
| stopSelf() | ||
| } |
There was a problem hiding this comment.
stopSelf() 호출 후 Flow 수집이 계속될 수 있습니다.
stopSelf()는 비동기적으로 서비스를 종료하며, onDestroy()가 호출될 때까지 시간이 걸립니다. 그 사이에 collect 블록이 계속 실행되어 추가 result를 처리하고 triggerAlarm()을 중복 호출할 수 있습니다.
🛡️ 제안: stopSelf() 전에 Job을 즉시 취소
alarmRepository.updateNearestNode(result)
if (result.remaining in 0..alarmInfo.stopsBeforeAlarm) {
alarmRepository.triggerAlarm()
+ locationJob?.cancel()
stopSelf()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt`
around lines 97 - 100, The issue is that stopSelf() asynchronously terminates
the service while the Flow collection continues, potentially causing multiple
triggerAlarm() calls. To fix this, cancel the Job that is managing the Flow
collection immediately before calling stopSelf(). This ensures the collect block
stops executing and prevents duplicate alarm triggers. Locate where the Job is
created for the Flow collection (likely in a lifecycleScope or separate job
variable) and call cancel() on it before the stopSelf() call in the conditional
block where result.remaining is in the range 0..alarmInfo.stopsBeforeAlarm.
Comment on lines
+34
to
+54
| @SuppressLint("MissingPermission") | ||
| override fun observeLocation(): Flow<Coordinate> = callbackFlow { | ||
| val request = LocationRequest.Builder(Priority.PRIORITY_HIGH_ACCURACY, LOCATION_INTERVAL_MS) | ||
| .setMinUpdateIntervalMillis(LOCATION_MIN_INTERVAL_MS) | ||
| .build() | ||
|
|
||
| val callback = object : LocationCallback() { | ||
| override fun onLocationResult(result: LocationResult) { | ||
| result.lastLocation?.let { location -> | ||
| Timber.d("위치 수신: lat=${location.latitude}, lon=${location.longitude}, accuracy=${location.accuracy}m") | ||
| trySend(Coordinate(location.latitude, location.longitude)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fusedClient.requestLocationUpdates(request, callback, Looper.getMainLooper()) | ||
|
|
||
| awaitClose { | ||
| fusedClient.removeLocationUpdates(callback) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# observeLocation() 호출 지점을 찾아 권한 검증 여부 확인
rg -nP --type=kt -C5 'observeLocation\s*\(' -g '!LocationRepositoryImpl.kt'Repository: mj010504/StopReminder
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for observeLocation() calls using correct rg syntax
rg -n 'observeLocation\s*\(' --type-list | head -20Repository: mj010504/StopReminder
Length of output: 455
🏁 Script executed:
#!/bin/bash
# List kotlin files first to understand structure
find . -type f -name "*.kt" | head -30Repository: mj010504/StopReminder
Length of output: 2291
🏁 Script executed:
#!/bin/bash
# Search for observeLocation() calls in kotlin files
rg -n 'observeLocation' -A3 -B3 --glob='*.kt' | head -100Repository: mj010504/StopReminder
Length of output: 2738
🏁 Script executed:
#!/bin/bash
# Check if LocationRepositoryImpl receives context and how permissions are validated
cat -n core/data/src/main/java/com/choiminjun/data/repository/LocationRepositoryImpl.kt | head -60Repository: mj010504/StopReminder
Length of output: 2835
🏁 Script executed:
#!/bin/bash
# Search for permission check patterns in the codebase
rg -n 'checkSelfPermission|ContextCompat\.checkSelfPermission|ACCESS_FINE_LOCATION|ACCESS_COARSE_LOCATION' --glob='*.kt' | head -50Repository: mj010504/StopReminder
Length of output: 1012
🏁 Script executed:
#!/bin/bash
# Find all files that call observeLocation()
rg -l 'observeLocation' --glob='*.kt' | grep -v LocationRepositoryImplRepository: mj010504/StopReminder
Length of output: 146
🏁 Script executed:
#!/bin/bash
# Get detailed context around observeLocation() calls
rg -B10 -A5 'observeLocation\(\)' --glob='*.kt' | head -150Repository: mj010504/StopReminder
Length of output: 5536
🏁 Script executed:
#!/bin/bash
# Check AlarmSettingScreen more carefully to see the permission flow
cat -n feature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingScreen.kt | sed -n '90,160p'Repository: mj010504/StopReminder
Length of output: 3362
🏁 Script executed:
#!/bin/bash
# Find where observeNearestNode or observeLocation is called from
rg -B5 -A10 'observeNearestNode\(' --glob='*.kt'Repository: mj010504/StopReminder
Length of output: 13186
🏁 Script executed:
#!/bin/bash
# Check the ViewModel that handles AlarmSettingIntent to see permission flow
rg -l 'AlarmSettingIntent' --glob='*.kt'Repository: mj010504/StopReminder
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Find onAlarmSet function to see where location is actually used
rg -B5 -A10 'onAlarmSet' --glob='*.kt' | head -100Repository: mj010504/StopReminder
Length of output: 10563
🏁 Script executed:
#!/bin/bash
# Check AlarmForegroundService and where startLocationTracking is called
cat -n app/src/main/java/com/choiminjun/stopreminder/service/AlarmForegroundService.kt | head -100Repository: mj010504/StopReminder
Length of output: 4307
🏁 Script executed:
#!/bin/bash
# Check AlarmForegroundService more - find where startLocationTracking is called
rg -B10 -A5 'startLocationTracking' --glob='*.kt'Repository: mj010504/StopReminder
Length of output: 5010
🏁 Script executed:
#!/bin/bash
# Check if observeLocation is ever called directly (not through observeNearestNode)
rg -n '\.observeLocation\(' --glob='*.kt'Repository: mj010504/StopReminder
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check how AlarmForegroundService is started - find all Intent/startService calls
rg -n 'startService|Intent.*AlarmForegroundService' --glob='*.kt'Repository: mj010504/StopReminder
Length of output: 387
🏁 Script executed:
#!/bin/bash
# Check if there's a manifest file and any exported services
find . -name "AndroidManifest.xml" -type fRepository: mj010504/StopReminder
Length of output: 545
🏁 Script executed:
#!/bin/bash
# Verify the permission validation flow one more time in AlarmSettingScreen
cat -n feature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingScreen.kt | sed -n '130,160p'Repository: mj010504/StopReminder
Length of output: 1781
🏁 Script executed:
#!/bin/bash
# Check the complete AlarmForegroundService manifest to see if it requires permissions
cat -n ./app/src/main/AndroidManifest.xml | grep -A5 -B5 'AlarmForegroundService'Repository: mj010504/StopReminder
Length of output: 552
🏁 Script executed:
#!/bin/bash
# Check if there are any direct calls to observeNearestNode from LocationRepository outside the main flow
rg -n 'observeNearestNode\(' --glob='*.kt' -B3 | grep -v '^--$'Repository: mj010504/StopReminder
Length of output: 4647
🏁 Script executed:
#!/bin/bash
# Check if LocationRepository interface has any KDoc/documentation
cat -n core/domain/src/main/java/com/choiminjun/domain/repository/LocationRepository.ktRepository: mj010504/StopReminder
Length of output: 631
런타임 권한 검증이 없는 상태로 방어적 코딩 패턴 위반.
observeLocation()은 @SuppressLint("MissingPermission")으로 경고를 억제하고 있지만, 메서드 자체에 런타임 권한 검증이 없습니다. 현재는 UI 레이어(AlarmSettingScreen)에서 호출 전에 권한 검증을 수행하고 있으나, 리포지토리가 권한을 확인하지 않으면 향후 새로운 호출자가 권한 검증 없이 이 메서드를 호출할 경우 SecurityException이 발생할 위험이 있습니다.
리포지토리 레이어에서는 최소한 다음 중 하나를 구현해야 합니다:
- 메서드 내에서 런타임 권한 검증 추가
- 인터페이스와 구현에 권한 필수 조건을 명시하는 KDoc 추가
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/data/src/main/java/com/choiminjun/data/repository/LocationRepositoryImpl.kt`
around lines 34 - 54, The observeLocation() method in LocationRepositoryImpl
suppresses the MissingPermission lint warning but lacks runtime permission
validation, creating a defensive coding gap where future callers could trigger
SecurityException. Either add runtime permission validation using
ActivityCompat.checkSelfPermission() to verify
android.permission.ACCESS_FINE_LOCATION and
android.permission.ACCESS_COARSE_LOCATION before calling
fusedClient.requestLocationUpdates(), or add comprehensive KDoc documentation to
the observeLocation() method and its interface declaration explicitly stating
that the caller must ensure required location permissions (ACCESS_FINE_LOCATION
and ACCESS_COARSE_LOCATION) are granted before invoking this method.
Comment on lines
+61
to
+75
| val windowNodes = nodes.subList(boardingIndex, destIndex + 1).toMutableList() | ||
| var windowStart = boardingIndex | ||
|
|
||
| Timber.d("노드 윈도우 초기화: boardingIndex=$boardingIndex, destIndex=$destIndex, windowSize=${windowNodes.size}") | ||
|
|
||
| emitAll( | ||
| observeLocation().mapNotNull { coord -> | ||
| if (windowNodes.isEmpty()) return@mapNotNull null | ||
|
|
||
| val nearestIdx = windowNodes.indices.minByOrNull { i -> | ||
| calculateDistance( | ||
| coord.latitude, coord.longitude, | ||
| windowNodes[i].latitude!!, windowNodes[i].longitude!!, | ||
| ) | ||
| } ?: return@mapNotNull null |
There was a problem hiding this comment.
인덱스 범위 검증 누락 및 null 좌표로 인한 NPE 위험.
두 가지 크리티컬 이슈가 있습니다:
-
Line 61:
nodes.subList(boardingIndex, destIndex + 1)이 유효하지 않은 인덱스(음수, 범위 초과, boardingIndex > destIndex)를 받으면IndexOutOfBoundsException이 발생합니다. -
Lines 73, 88-89:
windowNodes[i].latitude!!,longitude!!연산자는 좌표가 null인 경우 NPE를 발생시킵니다.BusNode의 위도/경도가 nullable 타입이므로 null 처리가 필수입니다.
🐛 인덱스 검증 및 null 처리 추가
override fun observeNearestNode(
nodes: List<BusNode>,
boardingIndex: Int,
destIndex: Int,
): Flow<NearestNodeResult> = flow {
+ require(boardingIndex >= 0) { "boardingIndex must be non-negative" }
+ require(destIndex < nodes.size) { "destIndex must be within nodes range" }
+ require(boardingIndex <= destIndex) { "boardingIndex must be <= destIndex" }
+
- val windowNodes = nodes.subList(boardingIndex, destIndex + 1).toMutableList()
+ val windowNodes = nodes.subList(boardingIndex, destIndex + 1)
+ .filter { it.latitude != null && it.longitude != null }
+ .toMutableList()
+
+ if (windowNodes.isEmpty()) {
+ Timber.e("No nodes with valid coordinates in range")
+ return@flow
+ }
var windowStart = boardingIndex
Timber.d("노드 윈도우 초기화: boardingIndex=$boardingIndex, destIndex=$destIndex, windowSize=${windowNodes.size}")
emitAll(
observeLocation().mapNotNull { coord ->
if (windowNodes.isEmpty()) return@mapNotNull null
val nearestIdx = windowNodes.indices.minByOrNull { i ->
calculateDistance(
coord.latitude, coord.longitude,
- windowNodes[i].latitude!!, windowNodes[i].longitude!!,
+ windowNodes[i].latitude!!, windowNodes[i].longitude!!, // 이제 안전함 (filter로 검증됨)
)
} ?: return@mapNotNull null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/data/src/main/java/com/choiminjun/data/repository/LocationRepositoryImpl.kt`
around lines 61 - 75, The code has two critical issues: (1) The nodes.subList
call at line 61 lacks validation for boardingIndex and destIndex parameters,
which can cause IndexOutOfBoundsException if they are negative, out of range, or
boardingIndex exceeds destIndex. Add bounds checking before the subList call to
ensure both indices are within valid range and boardingIndex is less than or
equal to destIndex. (2) The !! operators used on windowNodes[i].latitude and
windowNodes[i].longitude assume these nullable fields are never null, but they
can be, causing NPE. Replace the non-null assertion operators with safe null
handling such as using filterNotNull, safe let expressions, or proper null
checks to handle cases where coordinates might be null and skip processing those
nodes accordingly.
| ) { | ||
| operator fun invoke(alarmInfo: AlarmInfo): Flow<NearestNodeResult> = | ||
| observeNearestNode(alarmInfo.routeId, alarmInfo.boardingNodeId, alarmInfo.destNodeId) | ||
| .filter { it.remaining in 0..alarmInfo.stopsBeforeAlarm } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# AlarmInfo 데이터 클래스 정의 확인
ast-grep --pattern 'data class AlarmInfo($$$)'
# AlarmSetting 화면에서 stopsBeforeAlarm 입력 검증 확인
rg -n -A 5 'stopsBeforeAlarm' --glob '*AlarmSetting*'Repository: mj010504/StopReminder
Length of output: 1893
🏁 Script executed:
# Find selectedStopsBefore definition and usage
rg -n 'selectedStopsBefore'
# Check AlarmSettingViewModel for validation logic
rg -n -B 5 -A 10 'selectedStopsBefore' feature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingViewModel.kt
# Look at ObserveAlarmTriggerUseCase context
cat -n core/domain/src/main/java/com/choiminjun/domain/usecase/ObserveAlarmTriggerUseCase.kt
# Search for UI input field constraints (Android Compose or XML)
fd -t f '\.(kt|xml)$' feature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting | head -20Repository: mj010504/StopReminder
Length of output: 3578
🏁 Script executed:
# Read the AlarmSettingScreen around the stops selection UI
sed -n '530,600p' feature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingScreen.kt
# Also search for where the stops list is defined
rg -n 'stops\s*=' feature/alarm/src/main/java/com/choiminjun/alarm/alarmsetting/AlarmSettingScreen.kt | head -20
# Check for any test cases that might reveal expected behavior
fd -type f -name '*Test*.kt' feature/alarm | xargs rg -l 'ObserveAlarmTriggerUseCase\|AlarmSetting' 2>/dev/null || echo "No test files found"Repository: mj010504/StopReminder
Length of output: 3065
stopsBeforeAlarm 음수 검증 추가 권장
현재 UI는 AlarmConfirmBottomSheet에서 1~3 범위로만 선택을 제한하고 있어 음수 입력의 실제 위험은 낮습니다. 다만 SelectStopsBefore intent에서 검증 없이 직접 값을 복사하므로, 테스트나 데이터 복원 과정에서 예상 범위 밖의 값이 전달될 경우 ObserveAlarmTriggerUseCase 라인 15의 필터가 의도하지 않게 모든 항목을 제외할 수 있습니다. AlarmSettingViewModel에서 selectedStopsBefore를 1 이상의 양수로 검증하는 로직을 추가하는 것이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/domain/src/main/java/com/choiminjun/domain/usecase/ObserveAlarmTriggerUseCase.kt`
at line 15, The filter expression in ObserveAlarmTriggerUseCase uses a range
check with `alarmInfo.stopsBeforeAlarm` which could behave unexpectedly if
negative values are passed. Add validation in AlarmSettingViewModel to ensure
`selectedStopsBefore` is always validated as a positive number (greater than or
equal to 1) before it is used in the SelectStopsBefore intent. This prevents
negative or out-of-range values from reaching the filter logic at line 15 and
ensures the range condition works as intended.
| val boardingIndex = allNodes.indexOfFirst { it.nodeId == boardingNodeId } | ||
| val destIndex = allNodes.indexOfFirst { it.nodeId == destNodeId } | ||
|
|
||
| if (boardingIndex == -1 || destIndex == -1 || boardingIndex >= destIndex) return@flow |
There was a problem hiding this comment.
유효하지 않은 인덱스에 대한 로깅을 추가하세요.
boardingIndex 또는 destIndex가 유효하지 않을 때 flow가 조용히 완료됩니다. AlarmForegroundService의 재시도 루프가 이를 처리하지만, 디버깅을 위해 경고 로그를 추가하는 것이 좋습니다.
📝 로깅 추가 제안
+ if (boardingIndex == -1) {
+ Timber.w("Boarding node not found: boardingNodeId=$boardingNodeId, routeId=$routeId")
+ return@flow
+ }
+ if (destIndex == -1) {
+ Timber.w("Destination node not found: destNodeId=$destNodeId, routeId=$routeId")
+ return@flow
+ }
+ if (boardingIndex >= destIndex) {
+ Timber.w("Invalid node order: boardingIndex=$boardingIndex >= destIndex=$destIndex")
+ return@flow
+ }
- if (boardingIndex == -1 || destIndex == -1 || boardingIndex >= destIndex) return@flow🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@core/domain/src/main/java/com/choiminjun/domain/usecase/ObserveNearestNodeUseCase.kt`
at line 21, The condition checking for invalid indices (boardingIndex == -1,
destIndex == -1, or boardingIndex >= destIndex) in ObserveNearestNodeUseCase
silently returns from the flow without logging. Add a warning log statement
before the return@flow that includes the actual values of boardingIndex and
destIndex to help with debugging when these invalid conditions occur. This will
provide visibility into why the flow terminated without changing the existing
control flow logic.
| .fillMaxWidth() | ||
| .padding(horizontal = Spacing.space20), | ||
| ) { | ||
| itemsIndexed(displayNodes, key = { _, name -> name }) { index, nodeName -> |
There was a problem hiding this comment.
LazyColumn 키로 정류장 이름을 사용하면 중복 시 충돌 위험이 있습니다.
itemsIndexed의 key 파라미터로 nodeName을 사용하고 있는데, 실제 버스 노선에는 동일한 이름의 정류장이 여러 번 나타날 수 있습니다. 중복된 키는 Compose 재구성 시 UI 오류나 런타임 예외를 발생시킬 수 있습니다.
index를 키로 사용하거나, 고유성을 보장하기 위해 index와 name을 조합하는 것을 권장합니다.
🔧 중복 키 문제를 해결하는 수정안
- itemsIndexed(displayNodes, key = { _, name -> name }) { index, nodeName ->
+ itemsIndexed(displayNodes, key = { index, _ -> index }) { index, nodeName ->또는 더 안정적인 조합 키:
- itemsIndexed(displayNodes, key = { _, name -> name }) { index, nodeName ->
+ itemsIndexed(displayNodes, key = { index, name -> "$index-$name" }) { index, nodeName ->📝 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.
Suggested change
| itemsIndexed(displayNodes, key = { _, name -> name }) { index, nodeName -> | |
| itemsIndexed(displayNodes, key = { index, _ -> index }) { index, nodeName -> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorScreen.kt`
at line 188, The itemsIndexed function in AlarmMonitorScreen.kt uses nodeName as
the key parameter, but since bus routes can have duplicate station names, this
creates a key collision risk during Compose recomposition that may cause UI
errors or runtime exceptions. Replace the key lambda in the itemsIndexed call to
use a combination that ensures uniqueness, such as combining the index with the
nodeName (for example "index-nodeName") or simply using the index value alone if
the display order is stable and unique.
Comment on lines
+35
to
+42
| if (alarmInfo.isTriggered) { | ||
| postSideEffect(AlarmMonitorSideEffect.AlarmTriggered) | ||
| } else { | ||
| reduce { copy(alarmInfo = alarmInfo) } | ||
| if (state.value.routeNodeNames.isEmpty() && alarmInfo.routeId.isNotBlank()) { | ||
| loadRouteNodes(alarmInfo) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: AlarmDataSource에서 isTriggered 상태 관리 확인
rg -nP -C5 'isTriggered' --type=kt --glob '*AlarmDataSource*'Repository: mj010504/StopReminder
Length of output: 92
🏁 Script executed:
find . -type f -name "*.kt" | grep -E "(AlarmMonitorViewModel|AlarmDataSource)" | head -20Repository: mj010504/StopReminder
Length of output: 236
🏁 Script executed:
find . -type f -name "AlarmMonitorViewModel.kt" -o -name "*AlarmDataSource*.kt" | head -10Repository: mj010504/StopReminder
Length of output: 236
🏁 Script executed:
rg -n 'isTriggered' --type-list | grep -i kotlinRepository: mj010504/StopReminder
Length of output: 455
🏁 Script executed:
rg -n 'isTriggered' . | head -30Repository: mj010504/StopReminder
Length of output: 586
🏁 Script executed:
cat -n ./core/datastore/src/main/java/com/choiminjun/datastore/source/AlarmDataSource.ktRepository: mj010504/StopReminder
Length of output: 4000
🏁 Script executed:
cat -n ./feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorViewModel.ktRepository: mj010504/StopReminder
Length of output: 3677
🏁 Script executed:
cat -n ./core/domain/src/main/java/com/choiminjun/domain/model/alarm/AlarmInfo.ktRepository: mj010504/StopReminder
Length of output: 493
observeAlarm()에서 isTriggered 상태에 따른 비대칭적 상태 업데이트 로직을 정정하세요.
AlarmDataSource는 clearAlarm() 시 모든 preferences를 초기화하므로 isTriggered는 false로 복구됩니다(line 59-60).
문제점: observeAlarm() 메서드에서 isTriggered=true일 때 사이드 이펙트만 발생하고 상태는 업데이트되지 않습니다(line 36). 반대로 isTriggered=false일 때만 상태가 업데이트됩니다(line 38).
만약 화면이 빠르게 재진입되는 경우, 다음 시퀀스가 발생할 수 있습니다:
isTriggered=true발생 → 사이드 이펙트만 발생, 상태 미업데이트clearAlarm()호출 →isTriggered=false로 변경- 옵저버가 다시 발행 → 상태가 업데이트됨
이로 인해 네비게이션 이후 상태 업데이트가 발생할 수 있습니다. 사이드 이펙트 발생 시에도 상태를 업데이트하거나, 상태 초기화 로직을 명확히 하세요.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/alarm/src/main/java/com/choiminjun/alarm/alarmmonitor/AlarmMonitorViewModel.kt`
around lines 35 - 42, The observeAlarm() method in AlarmMonitorViewModel has
asymmetric state update logic where the if block (when isTriggered=true) only
posts a side effect without updating state, while the else block (when
isTriggered=false) updates state via reduce. To fix this, add a state update
using reduce in the if block where AlarmMonitorSideEffect.AlarmTriggered is
posted, so that the state is consistently updated along with the side effect.
This ensures that when the observer re-emits after clearAlarm() is called, the
state changes are properly captured without delayed updates after navigation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
개요
변경사항
도메인/UseCase
데이터 레이어
화면
서비스
버그 수정
Summary by CodeRabbit
릴리스 노트
New Features
Refactor