Merged
Conversation
📝 WalkthroughWalkthrough애플리케이션 전반에 푸시 알림 관련 기능이 추가되었습니다. 네트워크 계층에 NotificationService와 관련 요청/응답 모델이 도입되고, 도메인에 NotificationRepository 인터페이스와 데이터 계층의 DefaultNotificationRepository 구현이 추가되었습니다. 코어(notification) 모듈에 DI 모듈(notificationModule), 알림 딥링크 파싱·라우팅(NotificationRouter), 푸시 클릭 디스패처(NotificationLaunchDispatcher), 토큰 등록기(NotificationTokenRegistrar) 등이 도입되고, 앱 초기화와 네비게이션(AppNavHost, InitKoin) 및 온보딩·설정 화면에서 해당 기능들이 주입되어 사용됩니다. 런처 아이콘 및 리소스 변경과 알림 아이콘 교체도 포함됩니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml (1)
2-5:ic_launcher.xml과 동일한 구성입니다.
<monochrome>누락 이슈는ic_launcher.xml에 남긴 코멘트와 동일하게 적용됩니다. 두 파일 모두 수정이 필요합니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml` around lines 2 - 5, The round adaptive icon file is missing a <monochrome> element like the other launcher XML; update ic_launcher_round.xml to include a <monochrome android:drawable="@mipmap/ic_launcher_monochrome"/> (matching the entry in ic_launcher.xml) inside the <adaptive-icon> tag so both adaptive icons provide a monochrome drawable for legacy/monochrome rendering.
🧹 Nitpick comments (9)
app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml (1)
2-5: Android 13 Material You 테마 아이콘 지원 추가 고려현재 구현은 완벽하게 작동합니다. 다만 Android 13(API 33+)의 Material You 기능을 활용하면, 사용자 기기의 테마 색상에 맞춰 앱 아이콘이 자동으로 변색되는 경험을 제공할 수 있습니다. 이를 위해서는
<monochrome>요소가 필요합니다.
<monochrome>요소를 추가해도 API 33 미만 기기에서는 무시되므로 하위 호환성에 영향이 없습니다. 다만, 현재 모노크롬 전용 드로어블 리소스가 없으므로, 추가하기 전에 다음을 검토해야 합니다:
- 모노크롬 아이콘으로 사용할 별도의 드로어블이 필요한가? (현재는
ic_launcher_foreground를 재사용하거나 새로운 리소스 생성 필요)- 디자인 시스템에서 Material You 지원을 계획하고 있는가?
앞으로 Material You 지원을 추가한다면, 아래와 같이 적용할 수 있습니다:
💡 개선 예시
<adaptive-icon xmlns:android="http://schemas.android.com/apk/res/android"> <background android:drawable="@mipmap/ic_launcher_background"/> <foreground android:drawable="@mipmap/ic_launcher_foreground"/> <monochrome android:drawable="@drawable/ic_launcher_monochrome"/> </adaptive-icon>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml` around lines 2 - 5, Add Material You monochrome support by updating the adaptive icon block to include a <monochrome> element and provide a monochrome drawable; specifically, add a <monochrome> android:drawable="@drawable/ic_launcher_monochrome"/> to the adaptive-icon element and create or reuse a mono drawable (e.g., ic_launcher_monochrome or reuse ic_launcher_foreground converted to monochrome) so Android 13+ can apply themed colors while older APIs ignore the element.core/notification/src/main/java/com/twix/notification/model/PushPayload.kt (1)
9-14: 매직 스트링을 상수로 추출하는 것을 고려해 보세요.
"title","body","deepLink"같은 키 문자열이 하드코딩되어 있습니다. 현재는 이 파일에서만 사용되지만, 서버에서 전송하는 키와 정확히 일치해야 하므로 오타 방지와 유지보수성을 위해companion object에 상수로 추출하면 좋습니다.♻️ 상수 추출 제안
data class PushPayload( val title: String?, val body: String?, val deepLink: String?, -) +) { + companion object { + const val KEY_TITLE = "title" + const val KEY_BODY = "body" + const val KEY_DEEP_LINK = "deepLink" + } +} fun Map<String, String>.toTwixPushPayload() = PushPayload( - title = this["title"], - body = this["body"], - deepLink = this["deepLink"], + title = this[PushPayload.KEY_TITLE], + body = this[PushPayload.KEY_BODY], + deepLink = this[PushPayload.KEY_DEEP_LINK], )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/notification/src/main/java/com/twix/notification/model/PushPayload.kt` around lines 9 - 14, Map key strings "title", "body", "deepLink" are hardcoded in the extension function Map<String, String>.toTwixPushPayload() which risks typos and maintenance issues; extract these keys into constants (e.g., companion object) and reference them in toTwixPushPayload() when constructing PushPayload (title = ..., body = ..., deepLink = ...), so other code can reuse the same keys and avoid magic strings.core/network/src/main/java/com/twix/network/service/NotificationService.kt (1)
18-21: DELETE 메서드에@Body를사용할 수 있지만, 더 표준적인 방식으로 개선 고려Ktorfit은 v2.1.0 이상에서 DELETE 메서드에
@Body를지원하므로 코드 자체는 정상 작동합니다. 다만 HTTP 스펙(RFC 7231)에서 "DELETE 요청의 바디는 정의된 의미론이 없으며, 일부 구현에서는 해당 요청을 거부할 수 있다"고 명시하고 있습니다.실제 문제: 프록시, 로드 밸런서(AWS ALB, Cloudflare 등), 또는 일부 HTTP 클라이언트가 DELETE의 바디를 무시하거나 제거할 수 있어, 프로덕션 환경에서 예상치 못한 오류가 발생할 가능성이 있습니다.
권장 대안 (선택 사항):
- URL 경로에 토큰 포함:
DELETE api/v1/notifications/fcm-token/{token}- POST를 사용한 의도 명확화:
POST api/v1/notifications/fcm-token:delete(Google API 스타일)- Query parameter 활용:
DELETE api/v1/notifications/fcm-token?token={token}현재 서버 인프라가 DELETE + body를 안정적으로 지원한다면 현재 구현을 유지해도 되지만, 더 광범위한 호환성이 필요하다면 위 대안 중 하나 검토를 권장합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/network/src/main/java/com/twix/network/service/NotificationService.kt` around lines 18 - 21, The deleteFcmToken method currently sends TokenRequest in the DELETE body (suspend fun deleteFcmToken(`@Body` request: TokenRequest)), which can be dropped by proxies; change the API to avoid a DELETE body — e.g., switch to a path param: update the annotation to `@DELETE`("api/v1/notifications/fcm-token/{token}") and change the signature to suspend fun deleteFcmToken(`@Path`("token") token: String), or alternatively convert the endpoint to a POST-based delete (e.g., `@POST`("api/v1/notifications/fcm-token:delete") with `@Body` TokenRequest) or use a query param (`@DELETE`("api/v1/notifications/fcm-token") suspend fun deleteFcmToken(`@Query`("token") token: String)); update NotificationService.deleteFcmToken and any callers to match the chosen approach.core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt (1)
65-67:LocalDate::parse는 ISO-8601 형식(yyyy-MM-dd)만 파싱합니다.서버에서 해당 형식으로 날짜를 전송하지 않으면
DateTimeParseException이 발생하고, 이는 외부catch(e: Exception)에서 잡혀null을 반환합니다. 현재는 에러 핸들링이 되어 있지만, 파라미터가 없을 때(→IllegalArgumentException)와 형식이 잘못되었을 때(→DateTimeParseException) 에러 로그 메시지가 다르게 출력되어 디버깅 시 혼선이 생길 수 있습니다.서버와의 날짜 포맷 계약(
yyyy-MM-dd)을 주석 또는 테스트로 명시해두면 좋겠습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt` around lines 65 - 67, The requireLocalDateQuery function currently uses LocalDate::parse which only accepts ISO-8601 (yyyy-MM-dd) and will raise DateTimeParseException that gets treated differently than missing-param IllegalArgumentException; update Uri.requireLocalDateQuery to catch DateTimeParseException around LocalDate.parse and rethrow a single IllegalArgumentException (including the original exception as cause) with a clear message that specifies the expected format (yyyy-MM-dd), so logs are consistent, and add a KDoc comment above requireLocalDateQuery documenting the date format contract and a unit test for both missing and malformed date cases to ensure behavior.core/notification/src/main/java/com/twix/notification/di/NotificationModule.kt (1)
17-18:get()호출에 명시적 타입 지정을 권장합니다.
NotificationTokenRegistrar,NotificationRouter모두 3개의 untypedget()로 의존성을 주입받고 있습니다. Koin이 생성자 파라미터 타입으로 자동 추론하므로 현재는 정상 동작하지만, 향후 생성자 파라미터 순서 변경 또는 동일 타입의 여러 빈이 등록될 경우 잘못된 의존성이 주입될 수 있습니다.♻️ 제안: 명시적 타입 지정으로 가독성 및 안전성 향상
- single { NotificationTokenRegistrar(get(), get(), get()) } - single<NotificationDeepLinkHandler> { NotificationRouter(get(), get(), get()) } + single { NotificationTokenRegistrar(get<IdProvider>(), get<NotificationService>(), get<NotificationRepository>()) } + single<NotificationDeepLinkHandler> { NotificationRouter(get<AppNavigator>(), get<NotificationDeepLinkParser>(), get<NotificationRepository>()) }실제 타입은 각 생성자 시그니처를 참고하여 대입해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/notification/src/main/java/com/twix/notification/di/NotificationModule.kt` around lines 17 - 18, 현재 single { NotificationTokenRegistrar(get(), get(), get()) } 및 single<NotificationDeepLinkHandler> { NotificationRouter(get(), get(), get()) } 에서 세 개의 untyped get() 호출을 사용하고 있으므로, 각 생성자 파라미터의 실제 타입을 확인하여 get<Type>() 형태로 명시적으로 교체하세요; 예를 들어 NotificationTokenRegistrar와 NotificationRouter의 생성자 시그니처에 맞춰 각각 get<SomeDependencyA>(), get<SomeDependencyB>(), get<SomeDependencyC>()처럼 변경해 정확한 빈이 주입되도록 수정하고, NotificationDeepLinkHandler 바인딩은 그대로 유지하되 내부의 get()들만 명시적 타입으로 바꿔 테스트로 주입이 올바른지 확인하세요.core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceIdProvider.kt (1)
44-48:clear()가 뮤텍스를 보유하지 않아 잠재적인 레이스 컨디션이 발생할 수 있습니다.
getOrCreateDeviceId()가 락 내부에서 새 ID 쓰기(updateData)를 실행하는 도중,clear()가 동시에updateData를 호출할 경우 DataStore의 내부 직렬화에 의해 다음 두 시나리오가 발생할 수 있습니다:
getOrCreate쓰기 →clear쓰기 순으로 실행: 호출자는"twix-A"를 반환받지만 DataStore에는 빈 값이 저장됩니다.clear쓰기 →getOrCreate쓰기 순으로 실행: 의도한 클리어가 무효화됩니다.로그아웃 중 동시 호출 가능성은 낮지만,
clear()도 동일한 뮤텍스를 통해 직렬화하는 것이 더 안전합니다.🔒 제안: `clear()`에 뮤텍스 적용
override suspend fun clear() { - dataStore.updateData { currentValue -> - currentValue.copy(deviceId = "") + mutex.withLock { + dataStore.updateData { currentValue -> + currentValue.copy(deviceId = "") + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceIdProvider.kt` around lines 44 - 48, clear() currently calls dataStore.updateData without acquiring the same mutex used in getOrCreateDeviceId(), causing potential races; modify clear() to acquire the shared mutex (the same one used in getOrCreateDeviceId(), e.g., deviceIdMutex or mutex) before calling dataStore.updateData(deviceId = ""), so both getOrCreateDeviceId() and clear() are serialized, ensuring updates to dataStore.updateData are atomic relative to each other and avoiding the described interleavings.core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceId.kt (1)
27-37:SerializationException발생 시defaultValue로 조용히 폴백하는 방식과withContext(Dispatchers.IO)중복 사용에 대해 개선을 제안합니다.두 가지 사항입니다:
withContext(Dispatchers.IO)중복: DataStore는 달리 지정하지 않는 한 모든 데이터 작업을Dispatchers.IO에서 수행합니다. 따라서readFrom/writeTo내부의withContext(Dispatchers.IO)는 불필요합니다. 공식 Android 문서의 Serializer 예제도withContext없이 작성되어 있습니다.
IOException미처리 및defaultValue조용한 반환:input.readBytes()는IOException을 던질 수 있는데 현재catch블록이SerializationException만 처리합니다.IOException발생 시 예외가 DataStore 레이어로 전파되어.dataFlow가 에러 상태가 됩니다. 또한 공식 예제는SerializationException을CorruptionException으로 래핑하여 DataStore가 corruption 처리 전략을 선택할 수 있도록 합니다. 현재 방식(조용한defaultValue반환)은 데이터 손상 여부를 숨길 수 있습니다.♻️ 제안: withContext 제거 및 예외 처리 강화
override suspend fun readFrom(input: InputStream): DeviceId = try { - withContext(Dispatchers.IO) { - json.decodeFromString( - deserializer = DeviceId.serializer(), - string = input.readBytes().decodeToString(), - ) - } + json.decodeFromString( + deserializer = DeviceId.serializer(), + string = input.readBytes().decodeToString(), + ) } catch (e: SerializationException) { - defaultValue + throw CorruptionException("DeviceId 데이터 역직렬화에 실패했습니다.", e) + } catch (e: IOException) { + throw CorruptionException("DeviceId 데이터 읽기에 실패했습니다.", e) }override suspend fun writeTo( t: DeviceId, output: OutputStream, ) { - withContext(Dispatchers.IO) { - output.write( - json - .encodeToString( - serializer = DeviceId.serializer(), - value = t, - ).encodeToByteArray(), - ) - } + output.write( + json + .encodeToString( + serializer = DeviceId.serializer(), + value = t, + ).encodeToByteArray(), + ) }
CorruptionException을 사용하면 DataStore가ReplaceFileCorruptionHandler등의 정책으로 손상 복구를 처리할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceId.kt` around lines 27 - 37, readFrom currently wraps input.readBytes() with an unnecessary withContext(Dispatchers.IO) and silently returns defaultValue on SerializationException while ignoring IOExceptions; remove the withContext call around input.readBytes(), catch IOException and rethrow it so DataStore sees IO failures, and on SerializationException wrap and throw a CorruptionException (rather than returning defaultValue) so DataStore can apply corruption handlers; keep DeviceId.serializer() usage and only return defaultValue where explicitly intended elsewhere.core/notification/src/main/java/com/twix/notification/channel/TwixNotificationChannelManager.kt (1)
10-21:managernull 안전 처리는 선택적 개선사항입니다.현재 코드는 정상 작동하지만, 방어 프로그래밍 관점에서 다음을 고려해볼 수 있습니다:
getSystemService(NotificationManager::class.java)는 Android 문서에 따르면 null을 반환할 수 있습니다. NotificationManager는 핵심 시스템 서비스라 실제로 null이 될 가능성은 매우 낮지만, 혹시 모를 상황에 대비하려면 null-check를 추가하는 것이 좋습니다.💡 선택적 개선: Null 안전 처리
fun ensureDefaultChannel() { val manager = context.getSystemService(NotificationManager::class.java) ?: return val channel = NotificationChannel( CHANNEL_DEFAULT, "일반 알림", NotificationManager.IMPORTANCE_HIGH, ).apply { description = "Twix 알림 채널" } manager.createNotificationChannel(channel) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/notification/src/main/java/com/twix/notification/channel/TwixNotificationChannelManager.kt` around lines 10 - 21, ensureDefaultChannel uses context.getSystemService(NotificationManager::class.java) without null-check; make it null-safe by checking the returned manager (e.g., val manager = context.getSystemService(... ) ?: return or if (manager == null) { /* log and return */ }) and then call manager.createNotificationChannel(channel) only when non-null (or use safe-call manager?.createNotificationChannel(channel)); update references in ensureDefaultChannel to use this guard and optionally log the null case.app/src/main/java/com/yapp/twix/main/MainActivity.kt (1)
35-95: 알림 권한 요청 결과에 대한 안내 UX가 비어 있습니다.
권한 거부 시 아무 피드백이 없으면 사용자가 알림이 왜 안 오는지 알기 어렵습니다. 결과 콜백에서 토스트/설정 이동 안내를 추가해보는 건 어떨까요?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/yapp/twix/main/MainActivity.kt` around lines 35 - 95, 현재 requestNotificationPermissionLauncher는 결과 콜백이 비어 있어서 권한 거부 시 UX 피드백이 없음; registerForActivityResult(ActivityResultContracts.RequestPermission())의 빈 콜백을 권한 결과(boolean isGranted)를 처리하도록 수정하고, requestNotificationPermissionIfNeeded에서 launch하기 전에 해당 런처를 사용하도록 유지하되 콜백에서 권한 허용 여부를 판별해 허용 시 간단 토스트(또는 ToastManager에 이벤트 전송)를 표시하고 거부 시(특히 다시 묻지 않음 선택 가능성 있는 경우) 설정 화면으로 이동할지 묻는 토스트/다이얼로그 안내를 띄우도록 구현하세요; 관련 식별자: requestNotificationPermissionLauncher, requestNotificationPermissionIfNeeded, ToastManager/ToastHost, 그리고 onCreate의 LaunchedEffect에서 호출되는 흐름을 참고해 UI 스레드에서 알림을 표시하거나 ToastManager로 이벤트를 보내도록 하세요.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/yapp/twix/service/TwixFirebaseMessagingService.kt`:
- Around line 40-50: The current logger.e call logs the entire push payload
(dataPayload/payload) at error level which may expose personal data; change it
to avoid logging full payload by either restricting this log to debug builds
(check BuildConfig.DEBUG) or logging only non-sensitive identifiers (e.g.,
message.messageId or dataPayload.id) and lower the level from error to
debug/info; update the logging call in TwixFirebaseMessagingService where
logger.e is used after constructing payload (using identifiers like dataPayload,
payload, logger.e, showSystemNotification) so only safe minimal info is emitted.
In `@core/datastore/build.gradle.kts`:
- Around line 11-16: The dependencies for projects.core.token and
projects.core.deviceContract are declared with implementation(...) but must be
changed to api(...) so their types are exposed to consumers; update the
dependency lines to api(projects.core.token) and
api(projects.core.deviceContract) so the public DI module dataStoreModule (which
registers single<TokenProvider> and single<IdProvider>) can be resolved by
modules that depend on this library.
In `@core/design-system/src/main/res/drawable/ic_app.xml`:
- Around line 14-126: The vector drawable ic_app.xml contains hardcoded color
literals (`#ffffff`, `#171717`, `#FFD5DE`, `#BEF0A3`) in multiple path attributes
(android:fillColor, android:strokeColor) which bypass theme tokens and break
dark-mode; replace each literal with named color resources (e.g.,
`@color/icon_fill_primary`, `@color/icon_stroke`, `@color/cheek_blush`,
`@color/leaf_tint`) and update references in the same paths (look for path entries
with android:fillColor/android:strokeColor), then add corresponding dark
variants in values-night/colors.xml (or map to Material3 theme tokens) to ensure
proper contrast in dark mode and keep the design system consistent.
In
`@core/navigation-contract/src/main/java/com/twix/navigation_contract/NotificationDeepLinkHandler.kt`:
- Around line 3-5: KDoc 주석의 모듈 경로가 잘못되어 있으니 NotificationDeepLinkHandler.kt 상단의
코멘트를 수정하여 구현체가 위치한 모듈을 ":core:notification"으로 변경하고, ":core:navigation"은 이 인터페이스를
사용하는 모듈(AppNavHost)임을 명확히 표기하세요; 참조 대상인 NotificationRouter 이름을 언급해 주석을 업데이트하면
됩니다.
In `@core/navigation/src/main/java/com/twix/navigation/AppNavHost.kt`:
- Around line 92-95: The toStatisticsEndedGoals() method currently throws a
NotImplementedError via TODO, causing a crash when called from GoalEnded deep
links; replace that TODO with a safe fallback navigation path: after calling
ensureMainStack(navController) attempt to navigate to the statistics-ended route
if it exists, otherwise navigate to the safe fallback (e.g., Home route) so the
app does not crash; ensure no exceptions are thrown and use the same
navController/route constants used elsewhere (refer to toStatisticsEndedGoals(),
ensureMainStack(navController), and your Home/Statistics route identifiers) so
the real route can be swapped in later.
In
`@feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt`:
- Around line 148-153: The initNotificationSettings() call is missing an onError
handler and uses hardcoded booleans; update initNotificationSettings() to pass
an onError lambda that delegates errors to BaseViewModel.handleError (or logs
and triggers a retry/UX feedback) in the launchResult call, and replace the
positional true, true, true in notificationRepository.initNotificationSettings
with named arguments or well-named constants (e.g., ENABLE_PUSH = true,
ENABLE_EMAIL = true, ENABLE_SMS = true or use named params like
initNotificationSettings(push = true, email = true, sms = true)) so the intent
is explicit; keep the success handler as-is.
- Around line 19-21: The call to initNotificationSettings() is currently in the
OnBoardingViewModel init block and runs at ViewModel creation; move this call so
it executes only after the final onboarding step succeeds by invoking
initNotificationSettings() inside the anniversarySetup() success handler (e.g.,
within anniversarySetup(...).onSuccess { ... } or the corresponding callback in
the anniversarySetup flow), remove it from init, and ensure any required
context/state used by initNotificationSettings() is available at that point.
---
Duplicate comments:
In `@app/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml`:
- Around line 2-5: The round adaptive icon file is missing a <monochrome>
element like the other launcher XML; update ic_launcher_round.xml to include a
<monochrome android:drawable="@mipmap/ic_launcher_monochrome"/> (matching the
entry in ic_launcher.xml) inside the <adaptive-icon> tag so both adaptive icons
provide a monochrome drawable for legacy/monochrome rendering.
---
Nitpick comments:
In `@app/src/main/java/com/yapp/twix/main/MainActivity.kt`:
- Around line 35-95: 현재 requestNotificationPermissionLauncher는 결과 콜백이 비어 있어서 권한
거부 시 UX 피드백이 없음;
registerForActivityResult(ActivityResultContracts.RequestPermission())의 빈 콜백을 권한
결과(boolean isGranted)를 처리하도록 수정하고, requestNotificationPermissionIfNeeded에서
launch하기 전에 해당 런처를 사용하도록 유지하되 콜백에서 권한 허용 여부를 판별해 허용 시 간단 토스트(또는 ToastManager에
이벤트 전송)를 표시하고 거부 시(특히 다시 묻지 않음 선택 가능성 있는 경우) 설정 화면으로 이동할지 묻는 토스트/다이얼로그 안내를 띄우도록
구현하세요; 관련 식별자: requestNotificationPermissionLauncher,
requestNotificationPermissionIfNeeded, ToastManager/ToastHost, 그리고 onCreate의
LaunchedEffect에서 호출되는 흐름을 참고해 UI 스레드에서 알림을 표시하거나 ToastManager로 이벤트를 보내도록 하세요.
In `@app/src/main/res/mipmap-anydpi-v26/ic_launcher.xml`:
- Around line 2-5: Add Material You monochrome support by updating the adaptive
icon block to include a <monochrome> element and provide a monochrome drawable;
specifically, add a <monochrome>
android:drawable="@drawable/ic_launcher_monochrome"/> to the adaptive-icon
element and create or reuse a mono drawable (e.g., ic_launcher_monochrome or
reuse ic_launcher_foreground converted to monochrome) so Android 13+ can apply
themed colors while older APIs ignore the element.
In `@core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceId.kt`:
- Around line 27-37: readFrom currently wraps input.readBytes() with an
unnecessary withContext(Dispatchers.IO) and silently returns defaultValue on
SerializationException while ignoring IOExceptions; remove the withContext call
around input.readBytes(), catch IOException and rethrow it so DataStore sees IO
failures, and on SerializationException wrap and throw a CorruptionException
(rather than returning defaultValue) so DataStore can apply corruption handlers;
keep DeviceId.serializer() usage and only return defaultValue where explicitly
intended elsewhere.
In
`@core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceIdProvider.kt`:
- Around line 44-48: clear() currently calls dataStore.updateData without
acquiring the same mutex used in getOrCreateDeviceId(), causing potential races;
modify clear() to acquire the shared mutex (the same one used in
getOrCreateDeviceId(), e.g., deviceIdMutex or mutex) before calling
dataStore.updateData(deviceId = ""), so both getOrCreateDeviceId() and clear()
are serialized, ensuring updates to dataStore.updateData are atomic relative to
each other and avoiding the described interleavings.
In `@core/network/src/main/java/com/twix/network/service/NotificationService.kt`:
- Around line 18-21: The deleteFcmToken method currently sends TokenRequest in
the DELETE body (suspend fun deleteFcmToken(`@Body` request: TokenRequest)), which
can be dropped by proxies; change the API to avoid a DELETE body — e.g., switch
to a path param: update the annotation to
`@DELETE`("api/v1/notifications/fcm-token/{token}") and change the signature to
suspend fun deleteFcmToken(`@Path`("token") token: String), or alternatively
convert the endpoint to a POST-based delete (e.g.,
`@POST`("api/v1/notifications/fcm-token:delete") with `@Body` TokenRequest) or use a
query param (`@DELETE`("api/v1/notifications/fcm-token") suspend fun
deleteFcmToken(`@Query`("token") token: String)); update
NotificationService.deleteFcmToken and any callers to match the chosen approach.
In
`@core/notification/src/main/java/com/twix/notification/channel/TwixNotificationChannelManager.kt`:
- Around line 10-21: ensureDefaultChannel uses
context.getSystemService(NotificationManager::class.java) without null-check;
make it null-safe by checking the returned manager (e.g., val manager =
context.getSystemService(... ) ?: return or if (manager == null) { /* log and
return */ }) and then call manager.createNotificationChannel(channel) only when
non-null (or use safe-call manager?.createNotificationChannel(channel)); update
references in ensureDefaultChannel to use this guard and optionally log the null
case.
In
`@core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt`:
- Around line 65-67: The requireLocalDateQuery function currently uses
LocalDate::parse which only accepts ISO-8601 (yyyy-MM-dd) and will raise
DateTimeParseException that gets treated differently than missing-param
IllegalArgumentException; update Uri.requireLocalDateQuery to catch
DateTimeParseException around LocalDate.parse and rethrow a single
IllegalArgumentException (including the original exception as cause) with a
clear message that specifies the expected format (yyyy-MM-dd), so logs are
consistent, and add a KDoc comment above requireLocalDateQuery documenting the
date format contract and a unit test for both missing and malformed date cases
to ensure behavior.
In
`@core/notification/src/main/java/com/twix/notification/di/NotificationModule.kt`:
- Around line 17-18: 현재 single { NotificationTokenRegistrar(get(), get(), get())
} 및 single<NotificationDeepLinkHandler> { NotificationRouter(get(), get(),
get()) } 에서 세 개의 untyped get() 호출을 사용하고 있으므로, 각 생성자 파라미터의 실제 타입을 확인하여
get<Type>() 형태로 명시적으로 교체하세요; 예를 들어 NotificationTokenRegistrar와
NotificationRouter의 생성자 시그니처에 맞춰 각각 get<SomeDependencyA>(),
get<SomeDependencyB>(), get<SomeDependencyC>()처럼 변경해 정확한 빈이 주입되도록 수정하고,
NotificationDeepLinkHandler 바인딩은 그대로 유지하되 내부의 get()들만 명시적 타입으로 바꿔 테스트로 주입이 올바른지
확인하세요.
In `@core/notification/src/main/java/com/twix/notification/model/PushPayload.kt`:
- Around line 9-14: Map key strings "title", "body", "deepLink" are hardcoded in
the extension function Map<String, String>.toTwixPushPayload() which risks typos
and maintenance issues; extract these keys into constants (e.g., companion
object) and reference them in toTwixPushPayload() when constructing PushPayload
(title = ..., body = ..., deepLink = ...), so other code can reuse the same keys
and avoid magic strings.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/src/main/ic_launcher-playstore.pngis excluded by!**/*.png
📒 Files selected for processing (77)
app/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/com/yapp/twix/TwixApplication.ktapp/src/main/java/com/yapp/twix/di/InitKoin.ktapp/src/main/java/com/yapp/twix/main/MainActivity.ktapp/src/main/java/com/yapp/twix/service/TwixFirebaseMessagingService.ktapp/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlapp/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xmlapp/src/main/res/mipmap-anydpi/ic_launcher.xmlapp/src/main/res/mipmap-anydpi/ic_launcher_round.xmlapp/src/main/res/mipmap-hdpi/ic_launcher.webpapp/src/main/res/mipmap-hdpi/ic_launcher_background.webpapp/src/main/res/mipmap-hdpi/ic_launcher_foreground.webpapp/src/main/res/mipmap-hdpi/ic_launcher_round.webpapp/src/main/res/mipmap-mdpi/ic_launcher.webpapp/src/main/res/mipmap-mdpi/ic_launcher_background.webpapp/src/main/res/mipmap-mdpi/ic_launcher_foreground.webpapp/src/main/res/mipmap-mdpi/ic_launcher_round.webpapp/src/main/res/mipmap-xhdpi/ic_launcher.webpapp/src/main/res/mipmap-xhdpi/ic_launcher_background.webpapp/src/main/res/mipmap-xhdpi/ic_launcher_foreground.webpapp/src/main/res/mipmap-xhdpi/ic_launcher_round.webpapp/src/main/res/mipmap-xxhdpi/ic_launcher.webpapp/src/main/res/mipmap-xxhdpi/ic_launcher_background.webpapp/src/main/res/mipmap-xxhdpi/ic_launcher_foreground.webpapp/src/main/res/mipmap-xxhdpi/ic_launcher_round.webpapp/src/main/res/mipmap-xxxhdpi/ic_launcher.webpapp/src/main/res/mipmap-xxxhdpi/ic_launcher_background.webpapp/src/main/res/mipmap-xxxhdpi/ic_launcher_foreground.webpapp/src/main/res/mipmap-xxxhdpi/ic_launcher_round.webpcore/datastore/build.gradle.ktscore/datastore/src/main/java/com/twix/datastore/DataStore.ktcore/datastore/src/main/java/com/twix/datastore/deviceid/DeviceId.ktcore/datastore/src/main/java/com/twix/datastore/deviceid/DeviceIdProvider.ktcore/datastore/src/main/java/com/twix/datastore/di/DataStoreModule.ktcore/design-system/src/main/res/drawable/ic_app.xmlcore/device-contract/.gitignorecore/device-contract/build.gradle.ktscore/device-contract/consumer-rules.procore/device-contract/proguard-rules.procore/device-contract/src/main/AndroidManifest.xmlcore/device-contract/src/main/java/com/twix/device_contract/IdProvider.ktcore/navigation-contract/.gitignorecore/navigation-contract/build.gradle.ktscore/navigation-contract/consumer-rules.procore/navigation-contract/proguard-rules.procore/navigation-contract/src/main/AndroidManifest.xmlcore/navigation-contract/src/main/java/com/twix/navigation_contract/AppNavigator.ktcore/navigation-contract/src/main/java/com/twix/navigation_contract/NotificationDeepLinkHandler.ktcore/navigation-contract/src/main/java/com/twix/navigation_contract/NotificationLaunchEventSource.ktcore/navigation/build.gradle.ktscore/navigation/src/main/java/com/twix/navigation/AppNavHost.ktcore/network/src/main/java/com/twix/network/di/ApiServiceModule.ktcore/network/src/main/java/com/twix/network/model/request/notification/InitNotificationSettingsRequest.ktcore/network/src/main/java/com/twix/network/model/request/notification/RegisterFcmTokenRequest.ktcore/network/src/main/java/com/twix/network/model/request/notification/TokenRequest.ktcore/network/src/main/java/com/twix/network/service/NotificationService.ktcore/notification/.gitignorecore/notification/build.gradle.ktscore/notification/consumer-rules.procore/notification/proguard-rules.procore/notification/src/main/AndroidManifest.xmlcore/notification/src/main/java/com/twix/notification/channel/TwixNotificationChannelManager.ktcore/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLink.ktcore/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.ktcore/notification/src/main/java/com/twix/notification/di/NotificationModule.ktcore/notification/src/main/java/com/twix/notification/model/PushPayload.ktcore/notification/src/main/java/com/twix/notification/routing/NotificationLaunchDispatcher.ktcore/notification/src/main/java/com/twix/notification/routing/NotificationRouter.ktcore/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.ktdata/src/main/java/com/twix/data/di/RepositoryModule.ktdata/src/main/java/com/twix/data/repository/DefaultNotificationRepository.ktdomain/src/main/java/com/twix/domain/repository/NotificationRepository.ktfeature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.ktfeature/settings/build.gradle.ktsfeature/settings/src/main/java/com/twix/settings/SettingsViewModel.ktsettings.gradle.kts
💤 Files with no reviewable changes (2)
- app/src/main/res/mipmap-anydpi/ic_launcher_round.xml
- app/src/main/res/mipmap-anydpi/ic_launcher.xml
...avigation-contract/src/main/java/com/twix/navigation_contract/NotificationDeepLinkHandler.kt
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
Show resolved
Hide resolved
feature/onboarding/src/main/java/com/twix/onboarding/vm/OnBoardingViewModel.kt
Show resolved
Hide resolved
chanho0908
requested changes
Feb 25, 2026
| when (result) { | ||
| is AppResult.Success -> logger.d { "FCM token 삭제 성공" } | ||
| is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" } | ||
| } |
Member
There was a problem hiding this comment.
코드를 좀 더 간결화할 수 있을 것 같아 !
when (val result = notificationRepository.deleteFcmToken(fcmToken)) {
is AppResult.Success -> logger.d { "FCM token 삭제 성공" }
is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" }
}
| fun unregisterCurrentToken() { | ||
| appScope.launch { | ||
| try { | ||
| val fcmToken = FirebaseMessaging.getInstance().token.await() |
Member
There was a problem hiding this comment.
FirebaseMessaging도 koin으로 생성자 주입할 수 있게 만들면 추후 테스트를 만들 때 편리할 것 같은데 어떻게 생각할까 ?
Member
Author
There was a problem hiding this comment.
FirebaseMessaging 자체를 koin에 그대로 넣어도 결국에는 sdk에 직접 의존하는 건 같아서 테스트 이점이 크게 있을 거 같지는 않긴 해요!
Member
Author
There was a problem hiding this comment.
테스트가 용이하게 바꾼다고 하면 fcm 토큰 제공해주는 인터페이스를 만들어서 한번 감싸는 게 좋을 거 같은데 이건 데모 이후에 작업해서 올릴게요
| launchResult( | ||
| block = { authRepository.logout() }, | ||
| onSuccess = { | ||
| tokenRegistrar.unregisterCurrentToken() |
Member
There was a problem hiding this comment.
authRepository.logout()를 호출하면 토큰들을 모두 clear하는데 혹시 unregisterCurrentToken는 토큰이 필요 없는 요청일까 ?
Member
Author
There was a problem hiding this comment.
이거는 제가 생각을 해봤는데 서버 로직을 바꾸는 쪽이 더 안정적일 거 같아서 서버 팀원들이랑 얘기해보겠습니다!
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt (1)
51-66: 공유 기기에 대한 보안을 위해 클라이언트 측 토큰 삭제 추가 검토현재 구현은 서버에서 FCM 토큰 매핑을 삭제하는데, 이는 Firebase 공식 가이드에서 권장하는 핵심 단계입니다. 다만 공유 기기의 보안 강화를 위해
FirebaseMessaging.getInstance().deleteToken().await()를 추가로 호출하는 것을 고려해볼 만합니다.개선이 필요한 이유:
- 서버 측 토큰 삭제만으로는 기기 내 로컬 토큰이 유지되어 공유 기기에서 계정 전환 시 위험이 있을 수 있습니다
- 클라이언트 측에서 명시적으로 토큰을 삭제하면 같은 기기의 다른 계정에서 푸시를 받을 가능성을 최소화할 수 있습니다
제안:
fun unregisterCurrentToken() { appScope.launch { try { val fcmToken = FirebaseMessaging.getInstance().token.await() FirebaseMessaging.getInstance().deleteToken().await() when (val result = notificationRepository.deleteFcmToken(fcmToken)) { is AppResult.Success -> logger.d { "FCM token 삭제 성공" } is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" } } } catch (ce: CancellationException) { throw ce } catch (e: Exception) { logger.e(e) { "FCM token 조회/삭제 실패" } } } }이 앱이 공유 기기를 주요 사용 시나리오로 지원하는 경우 권장됩니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt` around lines 51 - 66, In unregisterCurrentToken(), after obtaining the current FCM token via FirebaseMessaging.getInstance().token.await(), also call FirebaseMessaging.getInstance().deleteToken().await() before invoking notificationRepository.deleteFcmToken(fcmToken) so the local client token is removed as well as the server-side mapping; keep the existing try/catch (rethrow CancellationException) and logging flow (logger.d/logger.e) around the repository call, and ensure any exceptions from deleteToken() are caught by the same catch block so they are logged as "FCM token 조회/삭제 실패".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt`:
- Around line 51-66: In unregisterCurrentToken(), guard against an empty FCM
token returned by FirebaseMessaging.getInstance().token.await() before calling
notificationRepository.deleteFcmToken(fcmToken): after obtaining fcmToken, check
if fcmToken.isBlank() and if so log an informational/debug message via logger
(similar to registerInternal) and skip calling deleteFcmToken; otherwise proceed
to call notificationRepository.deleteFcmToken and handle
AppResult.Success/AppResult.Error as existing code does, preserving the
surrounding coroutine scope and exception handling.
- Around line 51-66: Change unregisterCurrentToken from a fire-and-forget
appScope.launch into a suspend function so callers can await completion: make
NotificationTokenRegistrar.unregisterCurrentToken a suspend fun, remove the
appScope.launch wrapper, perform val fcmToken =
FirebaseMessaging.getInstance().token.await() and call
notificationRepository.deleteFcmToken(fcmToken) directly, preserve the existing
when(result) logging for AppResult.Success and AppResult.Error, rethrow
CancellationException and propagate/handle other Exceptions via logger as
before; update callers (e.g., SettingsViewModel logout flow) to invoke
unregisterCurrentToken from a coroutine and consider using a timeout if you want
to bound UX latency.
---
Nitpick comments:
In
`@core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt`:
- Around line 51-66: In unregisterCurrentToken(), after obtaining the current
FCM token via FirebaseMessaging.getInstance().token.await(), also call
FirebaseMessaging.getInstance().deleteToken().await() before invoking
notificationRepository.deleteFcmToken(fcmToken) so the local client token is
removed as well as the server-side mapping; keep the existing try/catch (rethrow
CancellationException) and logging flow (logger.d/logger.e) around the
repository call, and ensure any exceptions from deleteToken() are caught by the
same catch block so they are logged as "FCM token 조회/삭제 실패".
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt
Comment on lines
+51
to
+66
| fun unregisterCurrentToken() { | ||
| appScope.launch { | ||
| try { | ||
| val fcmToken = FirebaseMessaging.getInstance().token.await() | ||
|
|
||
| when (val result = notificationRepository.deleteFcmToken(fcmToken)) { | ||
| is AppResult.Success -> logger.d { "FCM token 삭제 성공" } | ||
| is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" } | ||
| } | ||
| } catch (ce: CancellationException) { | ||
| throw ce | ||
| } catch (e: Exception) { | ||
| logger.e(e) { "FCM token 조회/삭제 실패" } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
unregisterCurrentToken에서 빈 토큰 검증이 누락되어 있습니다.
registerInternal(Line 69~72)에서는 fcmToken.isBlank() 검사를 수행하지만, unregisterCurrentToken에서는 동일한 방어 코드가 없습니다. FirebaseMessaging.getInstance().token.await()가 비정상적으로 빈 문자열을 반환하는 엣지 케이스에서 deleteFcmToken("")이 호출될 수 있습니다. 일관성 차원에서도 동일한 가드를 추가하는 게 좋습니다.
🛡️ 빈 토큰 검증 추가 제안
fun unregisterCurrentToken() {
appScope.launch {
try {
val fcmToken = FirebaseMessaging.getInstance().token.await()
+
+ if (fcmToken.isBlank()) {
+ logger.d { "FCM token이 비어있음, 삭제 생략" }
+ return@launch
+ }
+
when (val result = notificationRepository.deleteFcmToken(fcmToken)) {
is AppResult.Success -> logger.d { "FCM token 삭제 성공" }
is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" }
}
} catch (ce: CancellationException) {
throw ce
} catch (e: Exception) {
logger.e(e) { "FCM token 조회/삭제 실패" }
}
}
}📝 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
| fun unregisterCurrentToken() { | |
| appScope.launch { | |
| try { | |
| val fcmToken = FirebaseMessaging.getInstance().token.await() | |
| when (val result = notificationRepository.deleteFcmToken(fcmToken)) { | |
| is AppResult.Success -> logger.d { "FCM token 삭제 성공" } | |
| is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" } | |
| } | |
| } catch (ce: CancellationException) { | |
| throw ce | |
| } catch (e: Exception) { | |
| logger.e(e) { "FCM token 조회/삭제 실패" } | |
| } | |
| } | |
| } | |
| fun unregisterCurrentToken() { | |
| appScope.launch { | |
| try { | |
| val fcmToken = FirebaseMessaging.getInstance().token.await() | |
| if (fcmToken.isBlank()) { | |
| logger.d { "FCM token이 비어있음, 삭제 생략" } | |
| return@launch | |
| } | |
| when (val result = notificationRepository.deleteFcmToken(fcmToken)) { | |
| is AppResult.Success -> logger.d { "FCM token 삭제 성공" } | |
| is AppResult.Error -> logger.e { "FCM token 삭제 실패: ${result.error}" } | |
| } | |
| } catch (ce: CancellationException) { | |
| throw ce | |
| } catch (e: Exception) { | |
| logger.e(e) { "FCM token 조회/삭제 실패" } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt`
around lines 51 - 66, In unregisterCurrentToken(), guard against an empty FCM
token returned by FirebaseMessaging.getInstance().token.await() before calling
notificationRepository.deleteFcmToken(fcmToken): after obtaining fcmToken, check
if fcmToken.isBlank() and if so log an informational/debug message via logger
(similar to registerInternal) and skip calling deleteFcmToken; otherwise proceed
to call notificationRepository.deleteFcmToken and handle
AppResult.Success/AppResult.Error as existing code does, preserving the
surrounding coroutine scope and exception handling.
🧩 Analysis chain
🏁 Script executed:
rg -n "unregisterCurrentToken" --type kotlin -C 5Repository: YAPP-Github/Twix-Android
Length of output: 2738
🏁 Script executed:
rg -n "fun launchResult" --type kotlin -A 10Repository: YAPP-Github/Twix-Android
Length of output: 50
🏁 Script executed:
rg -n "val appScope|var appScope" --type kotlin -C 3Repository: YAPP-Github/Twix-Android
Length of output: 1872
🏁 Script executed:
rg -n "appScope.*=" --type kotlin | head -20Repository: YAPP-Github/Twix-Android
Length of output: 50
🏁 Script executed:
rg -n "fun.*launchResult|val.*launchResult" --type kotlin -B 2 -A 10Repository: YAPP-Github/Twix-Android
Length of output: 1363
🏁 Script executed:
rg -n "@Provides|@Singleton|fun.*appScope|provides.*appScope" --type kotlin -B 2 -A 5 | head -40Repository: YAPP-Github/Twix-Android
Length of output: 50
🏁 Script executed:
find . -name "*Module.kt" -o -name "*Provider.kt" | head -10Repository: YAPP-Github/Twix-Android
Length of output: 744
🏁 Script executed:
cat -n ./app/src/main/java/com/yapp/twix/di/AppModule.ktRepository: YAPP-Github/Twix-Android
Length of output: 592
🏁 Script executed:
# Check if there are any other calls to unregisterCurrentToken or similar token cleanup patterns
rg -n "deleteFcmToken|unregister" --type kotlin -C 2Repository: YAPP-Github/Twix-Android
Length of output: 3913
로그아웃 시 FCM 토큰 삭제가 확실하게 완료되도록 보장해야 합니다.
현재 unregisterCurrentToken()은 appScope.launch로 실행되는 fire-and-forget 패턴으로, SettingsViewModel의 로그아웃 흐름에서 호출된 직후 즉시 토스트 표시와 로그인 화면 전환이 일어납니다. 만약 이 과정에서 사용자가 앱을 종료하거나 화면 전환이 빠르게 진행되면, 네트워크 요청이 완료되지 않은 상태에서 중단될 수 있습니다. 그 결과 서버에 이전 사용자의 FCM 토큰이 남아 이미 로그아웃한 사용자에게 푸시 알림이 전송될 수 있다는 점이 우려됩니다.
suspend fun으로 변경하여 호출 시점에서 완료를 기다리도록 개선하는 방안을 고려해봐도 좋을 것 같습니다. 물론 로그아웃 화면 전환의 응답성에 영향을 줄 수 있으므로, 팀과 함께 신뢰성과 UX 사이의 트레이드오프를 논의한 후 진행하는 것을 추천합니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt`
around lines 51 - 66, Change unregisterCurrentToken from a fire-and-forget
appScope.launch into a suspend function so callers can await completion: make
NotificationTokenRegistrar.unregisterCurrentToken a suspend fun, remove the
appScope.launch wrapper, perform val fcmToken =
FirebaseMessaging.getInstance().token.await() and call
notificationRepository.deleteFcmToken(fcmToken) directly, preserve the existing
when(result) logging for AppResult.Success and AppResult.Error, rethrow
CancellationException and propagate/handle other Exceptions via logger as
before; update callers (e.g., SettingsViewModel logout flow) to invoke
unregisterCurrentToken from a coroutine and consider using a timeout if you want
to bound UX latency.
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
2 participants
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.
이슈 번호
작업내용
결과물
2026-02-24.11.00.22.mov
리뷰어에게 추가로 요구하는 사항 (선택)
앱 아이콘 변경했습니다!
그리고 제가 온보딩 뷰모델에 임시로 알림 권한 초기화하는 api 넣어뒀는데 이거 다이얼로그 띄워서 마케팅 수신 동의 처리하고, 유저 응답값을 서버에 보내도록 수정해주세요!