Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough이 PR은 FCM 기반 푸시 알림 기능을 추가합니다. app 모듈에 Koin DI, 권한 요청 및 FCM 서비스 등록을 추가하고 TwixFirebaseMessagingService를 선언했습니다. core에 notification, navigation-contract, device-contract 모듈을 새로 도입해 알림 채널 관리자, 푸시 페이로드/딥링크 파서, 딥링크 라우터, 알림 발신 디스패처, 토큰 등록기, DeviceId DataStore 및 IdProvider 구현과 관련 DI 모듈들을 추가·연결했습니다. AppNavHost와 MainActivity에 알림 딥링크 처리 흐름이 통합되었습니다. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 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
🧹 Nitpick comments (5)
core/navigation-contract/build.gradle.kts (1)
10-10: 프로젝트 전체에서 사용하는 타입 세이프 접근자(projects.*)와 표기 방식이 다릅니다.
core/notification/build.gradle.kts,core/navigation/build.gradle.kts등 이 PR의 다른 Gradle 파일들은 모두projects.core.ui방식을 사용하는데, 이 파일만 문자열 기반project(":core:ui")를 사용하고 있어 일관성이 깨집니다.♻️ 제안하는 수정
dependencies { - implementation(project(":core:ui")) + implementation(projects.core.ui) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/navigation-contract/build.gradle.kts` at line 10, Replace the string-based project reference implementation(project(":core:ui")) with the type-safe Gradle projects accessor used across the repo; update the dependency to use projects.core.ui so it matches the other build scripts (e.g., core/notification/build.gradle.kts) and preserves consistency in the notation.core/navigation-contract/src/main/java/com/twix/navigation_contract/NotificationLaunchEventSource.kt (1)
14-14:consumePendingDeepLink의expected파라미터 의도를 문서화하면 좋을 것 같습니다.현재 코드만 봐서는
expected가 "현재 값이 이 값과 일치할 때만 소비(compare-and-clear)" 하는 역할인지 단순히 소비할 딥링크를 명시하는 역할인지 파악하기 어렵습니다. 구현 코드를 별도로 확인해야 이해할 수 있어 계약(interface)으로서의 표현력이 떨어집니다.KDoc을 추가해 동작을 명확히 전달하는 것을 권장합니다.
📝 KDoc 추가 예시
- fun consumePendingDeepLink(expected: String? = null) + /** + * pendingDeepLink를 소비(null로 초기화)합니다. + * [expected]가 null이 아닌 경우, 현재 [pendingDeepLink] 값과 일치할 때만 소비합니다. + * 이를 통해 처리 중 다른 deepLink로 교체된 경우 의도치 않은 소비를 방지합니다. + */ + fun consumePendingDeepLink(expected: String? = null)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/navigation-contract/src/main/java/com/twix/navigation_contract/NotificationLaunchEventSource.kt` at line 14, 문제: consumePendingDeepLink의 expected 파라미터 의도가 인터페이스 문서에 명확히 드러나지 않습니다. 수정: NotificationLaunchEventSource의 consumePendingDeepLink(expected: String? = null) 함수에 KDoc을 추가하여 expected가 "비교-후-소비(compare-and-clear)" 동작을 하는지(즉 현재 보관된 딥링크가 expected와 일치할 때만 소비하는지), expected가 null일 때는 모든 대기 딥링크를 소비하는지, 반환값(예: 소비된 딥링크 또는 null)과 동시성/스레드 안전성 관련 보장 여부를 간단히 기술하세요. 함수명 consumePendingDeepLink과 파라미터 expected를 명시하여 어떤 조건에서 소비가 발생하는지 예시 한 줄로 명시하면 충분합니다.core/notification/src/main/java/com/twix/notification/routing/NotificationRouter.kt (1)
10-14:appScope파라미터가 현재 사용되지 않습니다.
markAsReadBestEffort가 완전히 주석 처리된 상태이므로appScope는 어디서도 참조되지 않습니다. 기능 구현 전까지는 YAGNI 원칙에 따라 제거하거나, 향후 구현 예정임을 명시하는 TODO를 추가하는 방향을 고려해보세요.♻️ 수정 제안 (구현 예정 명시 방식)
class NotificationRouter( private val parser: NotificationDeepLinkParser, -// private val notificationRepository: NotificationRepository, - private val appScope: CoroutineScope, + // TODO: 읽음 처리 구현 시 아래 파라미터 추가 + // private val notificationRepository: NotificationRepository, + // private val appScope: CoroutineScope, ) : NotificationDeepLinkHandler {🤖 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/routing/NotificationRouter.kt` around lines 10 - 14, NotificationRouter currently accepts an unused constructor parameter appScope because markAsReadBestEffort is fully commented out; either remove appScope from NotificationRouter's constructor and any callers to eliminate dead code, or if you plan to implement markAsReadBestEffort soon, restore a short TODO comment above appScope and the commented method (or add a KDoc `@todo`) clarifying intended future use so the unused parameter is intentional; reference the NotificationRouter class, the appScope constructor parameter, and the commented markAsReadBestEffort method when making the change.core/notification/src/main/java/com/twix/notification/routing/NotificationLaunchDispatcher.kt (1)
42-52:shouldIgnoreDuplicate가 상태를 변경하는 부수 효과를 가지고 있으며, 매직 넘버1500L이 사용됩니다.
shouldIgnore...이라는 이름은 순수한 조건 검사(predicate)를 암시하지만, 내부에서lastDispatchedDeepLink와lastDispatchedAtMillis를 갱신합니다. Command-Query Separation 관점에서 이름이 의도를 완전히 반영하지 못합니다. 또한1500L은 의미를 알기 어려운 매직 넘버입니다.♻️ 수정 제안
+ companion object { + const val ACTION_NOTIFICATION_CLICK = "com.twix.notification.ACTION_CLICK" + const val EXTRA_DEEP_LINK = "extra_deep_link" + const val EXTRA_FROM_PUSH_CLICK = "extra_from_push_click" + private const val DUPLICATE_DISPATCH_THRESHOLD_MS = 1_500L + } + - private fun shouldIgnoreDuplicate(deepLink: String): Boolean { + /** + * 중복 여부를 확인하고, 중복이 아닌 경우 마지막 디스패치 상태를 갱신합니다. + * `@return` 중복이면 true (무시해야 함), 새로운 이벤트면 false + */ + private fun checkDuplicateAndRecord(deepLink: String): Boolean { val now = System.currentTimeMillis() val isDuplicate = - lastDispatchedDeepLink == deepLink && (now - lastDispatchedAtMillis) < 1500L + lastDispatchedDeepLink == deepLink && + (now - lastDispatchedAtMillis) < DUPLICATE_DISPATCH_THRESHOLD_MS if (!isDuplicate) { lastDispatchedDeepLink = deepLink lastDispatchedAtMillis = now } return isDuplicate }호출부도 함께 변경:
- if (shouldIgnoreDuplicate(deepLink)) return + if (checkDuplicateAndRecord(deepLink)) return🤖 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/routing/NotificationLaunchDispatcher.kt` around lines 42 - 52, The method shouldIgnoreDuplicate has hidden side-effects (it mutates lastDispatchedDeepLink and lastDispatchedAtMillis) and uses the magic number 1500L; change this by extracting the duplication check into a pure predicate and a separate mutating method (or rename to clearly indicate mutation, e.g., isDuplicateAndUpdateLastDispatched) and replace 1500L with a named constant (e.g., DUPLICATE_WINDOW_MS) so intent is clear; update all callers to either call the pure predicate then call the mutator on non-duplicate, or to use the new renamed method if mutation-first behavior is intended, referencing shouldIgnoreDuplicate, lastDispatchedDeepLink, lastDispatchedAtMillis, and the 1500L occurrence when locating the changes.core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceId.kt (1)
27-37:SerializationException시defaultValue를 반환하면 데이터 손실이 무증상으로 발생할 수 있습니다.현재 구현은 역직렬화 실패 시 예외를 무시하고 빈
DeviceId()를 반환합니다. 이렇게 되면 저장된 디바이스 ID가 오염된 경우에도 조용히 새 UUID로 교체되어 디버깅이 어렵습니다. Android 공식 문서의 예시에서는SerializationException을CorruptionException으로 감싸서 throw하는 방식을 권장합니다.또한,
input.readBytes()는IOException을 던질 수 있는데, 이를 처리하지 않으면 DataStore의dataFlow가 에러를 방출합니다. Android 공식 문서에서도 DataStore를 읽을 때IOException을 별도로 처리하도록 안내하고 있습니다.디바이스 ID는 자동 복구가 허용된다면
IOException도 함께 처리하는 방향을 고려해보세요.♻️ 수정 제안 (두 가지 방식)
방식 A: 공식 문서 권장 방식 (CorruptionException 사용, IOException은 호출부에서 처리)
override suspend fun readFrom(input: InputStream): DeviceId = try { withContext(Dispatchers.IO) { json.decodeFromString( deserializer = DeviceId.serializer(), string = input.readBytes().decodeToString(), ) } - } catch (e: SerializationException) { - defaultValue + } catch (e: SerializationException) { + throw CorruptionException("DeviceId 데이터가 손상되었습니다.", e) }방식 B: 자동 복구 허용 (DeviceId 특성상 재생성이 허용되는 경우)
- } catch (e: SerializationException) { - defaultValue + } catch (e: IOException) { + defaultValue + } catch (e: SerializationException) { + defaultValue }🤖 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, In readFrom (DeviceId.readFrom) don’t silently swallow deserialization failures: catch SerializationException and rethrow it wrapped in an androidx.datastore.core.CorruptionException (preserving the original exception as the cause) instead of returning defaultValue; also explicitly handle IOExceptions from input.readBytes() — either rethrow the IOException so DataStore can surface it (official recommended behavior) or, if DeviceId is allowed to be auto-recovered, catch IOException and return defaultValue (Document this choice), and ensure the json.decodeFromString call still happens on Dispatchers.IO via withContext as currently implemented.
🤖 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 84-105: In PushPayload.resolveStableNotificationId(), don’t
silently swallow exceptions when parsing deepLink; catch the exception and
record it (include the exception message/stack and the failing deepLink value)
via the app’s logging/metrics facility (e.g., Log/Timber or your telemetry
client) before returning null so you can trace bad deep links — update the
try/catch around deepLink.toUri() in resolveStableNotificationId to log the
caught Exception and the deepLink string (use unique identifiers deepLink and
fromDeepLink in the log) rather than discarding it.
In `@core/navigation-contract/build.gradle.kts`:
- Around line 9-11: The project declares an unnecessary dependency on :core:ui
in the dependencies block (implementation(project(":core:ui"))) even though
navigation-contract interfaces (AppNavigator, NotificationDeepLinkHandler,
NotificationLaunchEventSource) use only standard types; remove the
implementation(project(":core:ui")) entry from build.gradle.kts so the contract
module remains lightweight and does not transitively pull :core:ui into its
consumers.
In `@core/navigation/src/main/java/com/twix/navigation/AppNavHost.kt`:
- Around line 92-95: The TODO in toStatisticsEndedGoals currently throws
NotImplementedError at runtime; replace the TODO with a safe fallback navigation
or guard: call ensureMainStack(navController) then perform a non-crashing action
(e.g., navigate to a safe default route such as the main statistics screen or
the home screen via navController.navigate(...) or simply return/no-op) and
optionally log the unexpected invocation; alternatively, block this deep link
upstream in the router if the route isn't supported. Ensure you update the
toStatisticsEndedGoals implementation (and keep ensureMainStack/navController
usage) so it never throws.
In `@core/notification/build.gradle.kts`:
- Line 5: Remove the google services Gradle plugin from the library module by
deleting the alias(libs.plugins.google.services) entry in core/notification's
build.gradle.kts; the google-services plugin must only be applied in the app
module (app/build.gradle.kts) so leave that file unchanged and ensure
notification module keeps only the Firebase dependency declarations (e.g.,
firebase-messaging BOM) without applying the plugin.
In
`@core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt`:
- Around line 55-57: In NotificationDeepLinkParser.kt update the catch block
that currently calls logger.e("notification deepLink 파싱에 실패했습니다.: $raw") so the
caught exception e is passed to the logger; change the logger invocation to the
project's standard form (e.g., logger.e(e) { "notification deepLink 파싱에 실패했습니다.:
$raw" } or logger.e("notification deepLink 파싱에 실패했습니다.: $raw", e)) so the stack
trace is recorded when parsing fails.
In
`@core/notification/src/main/java/com/twix/notification/routing/NotificationRouter.kt`:
- Around line 66-76: Uncomment and restore the best-effort background mark logic
inside markAsReadBestEffort, keeping the appScope.launch and try/catch
structure, but fix the logger call in the catch for Exception: replace the
invalid logger.w(it, "...") with the Kermit pattern logger.w(e) { "알림 읽음 처리 실패:
$notificationId" }, and ensure CancellationException is rethrown (catch (ce:
CancellationException) { throw ce }) while using the existing
notificationRepository.markNotification(notificationId).
In
`@core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt`:
- Around line 53-70: The registerInternal function currently never sends the FCM
token because the call to notificationRepository.registerFcmToken is commented
out; restore the registration by uncommenting (or re-adding) the call inside the
AppResult.Success<User> branch using userResult.data.id, deviceId from
deviceIdProvider.getOrCreateDeviceId(), and the fcmToken parameter, and ensure
any exceptions are handled/logged; if you intentionally defer sending the token,
add a clear TODO with an issue link inside registerInternal referencing why
notificationRepository.registerFcmToken is disabled so it’s tracked.
---
Nitpick comments:
In `@core/datastore/src/main/java/com/twix/datastore/deviceid/DeviceId.kt`:
- Around line 27-37: In readFrom (DeviceId.readFrom) don’t silently swallow
deserialization failures: catch SerializationException and rethrow it wrapped in
an androidx.datastore.core.CorruptionException (preserving the original
exception as the cause) instead of returning defaultValue; also explicitly
handle IOExceptions from input.readBytes() — either rethrow the IOException so
DataStore can surface it (official recommended behavior) or, if DeviceId is
allowed to be auto-recovered, catch IOException and return defaultValue
(Document this choice), and ensure the json.decodeFromString call still happens
on Dispatchers.IO via withContext as currently implemented.
In `@core/navigation-contract/build.gradle.kts`:
- Line 10: Replace the string-based project reference
implementation(project(":core:ui")) with the type-safe Gradle projects accessor
used across the repo; update the dependency to use projects.core.ui so it
matches the other build scripts (e.g., core/notification/build.gradle.kts) and
preserves consistency in the notation.
In
`@core/navigation-contract/src/main/java/com/twix/navigation_contract/NotificationLaunchEventSource.kt`:
- Line 14: 문제: consumePendingDeepLink의 expected 파라미터 의도가 인터페이스 문서에 명확히 드러나지
않습니다. 수정: NotificationLaunchEventSource의 consumePendingDeepLink(expected:
String? = null) 함수에 KDoc을 추가하여 expected가 "비교-후-소비(compare-and-clear)" 동작을 하는지(즉
현재 보관된 딥링크가 expected와 일치할 때만 소비하는지), expected가 null일 때는 모든 대기 딥링크를 소비하는지, 반환값(예:
소비된 딥링크 또는 null)과 동시성/스레드 안전성 관련 보장 여부를 간단히 기술하세요. 함수명 consumePendingDeepLink과
파라미터 expected를 명시하여 어떤 조건에서 소비가 발생하는지 예시 한 줄로 명시하면 충분합니다.
In
`@core/notification/src/main/java/com/twix/notification/routing/NotificationLaunchDispatcher.kt`:
- Around line 42-52: The method shouldIgnoreDuplicate has hidden side-effects
(it mutates lastDispatchedDeepLink and lastDispatchedAtMillis) and uses the
magic number 1500L; change this by extracting the duplication check into a pure
predicate and a separate mutating method (or rename to clearly indicate
mutation, e.g., isDuplicateAndUpdateLastDispatched) and replace 1500L with a
named constant (e.g., DUPLICATE_WINDOW_MS) so intent is clear; update all
callers to either call the pure predicate then call the mutator on
non-duplicate, or to use the new renamed method if mutation-first behavior is
intended, referencing shouldIgnoreDuplicate, lastDispatchedDeepLink,
lastDispatchedAtMillis, and the 1500L occurrence when locating the changes.
In
`@core/notification/src/main/java/com/twix/notification/routing/NotificationRouter.kt`:
- Around line 10-14: NotificationRouter currently accepts an unused constructor
parameter appScope because markAsReadBestEffort is fully commented out; either
remove appScope from NotificationRouter's constructor and any callers to
eliminate dead code, or if you plan to implement markAsReadBestEffort soon,
restore a short TODO comment above appScope and the commented method (or add a
KDoc `@todo`) clarifying intended future use so the unused parameter is
intentional; reference the NotificationRouter class, the appScope constructor
parameter, and the commented markAsReadBestEffort method when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
app/build.gradle.ktsapp/src/main/AndroidManifest.xmlapp/src/main/java/com/yapp/twix/TwixApplication.ktapp/src/main/java/com/yapp/twix/di/CoroutineModule.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.ktcore/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/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/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.ktsettings.gradle.kts
core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt
Show resolved
Hide resolved
core/notification/src/main/java/com/twix/notification/routing/NotificationRouter.kt
Show resolved
Hide resolved
core/notification/src/main/java/com/twix/notification/token/NotificationTokenRegistrar.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt (1)
55-57: 이전 리뷰 반영 확인 ✅이전 리뷰에서 지적된 예외
e가 로거에 전달되지 않는 문제가 정확히 수정되었습니다.logger.e(e) { "..." }형태로 스택 트레이스를 포함하도록 개선된 점이 좋습니다.🤖 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 55 - 57, The previous issue (missing exception being passed to the logger) has been fixed in NotificationDeepLinkParser by using logger.e(e) { "notification deepLink 파싱에 실패했습니다.: $raw" }; no further code changes are required—confirm the catch block in NotificationDeepLinkParser.kt still passes the caught Exception `e` into `logger.e(e) { ... }` to ensure stack traces are logged and keep the method returning null as currently implemented.
🧹 Nitpick comments (1)
core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt (1)
65-67:LocalDate::parse예외 타입 불일치 — 선택적 개선 제안
getQueryParameter(name)이null이면IllegalArgumentException("유효하지 않은 Date param입니다.: $name")이 발생하지만,null이 아닌 잘못된 형식(예:"not-a-date")이 들어오면LocalDate::parse가DateTimeParseException을 던져?: throw분기를 거치지 않습니다. 두 경우 모두 바깥의catch (e: Exception)에서 잡히므로 동작은 정상이나, 어느 파라미터가 문제인지에 대한 메시지 일관성이 떨어집니다.
logger.e(e) { ... }로 스택 트레이스가 이미 기록되기 때문에 디버깅에 치명적이지는 않지만, 헬퍼 함수의 의도 — "항상IllegalArgumentException으로 파라미터 이름을 포함한 메시지를 던진다" — 를 명확히 하려면 아래와 같이 개선할 수 있습니다.♻️ 예외 타입 일관성 개선 제안
private fun Uri.requireLocalDateQuery(name: String): LocalDate = - getQueryParameter(name)?.let(LocalDate::parse) - ?: throw IllegalArgumentException("유효하지 않은 Date param입니다.: $name") + getQueryParameter(name) + ?.let { runCatching { LocalDate.parse(it) }.getOrNull() } + ?: throw IllegalArgumentException("유효하지 않은 Date param입니다.: $name")🤖 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 helper requireLocalDateQuery currently lets LocalDate::parse throw DateTimeParseException for malformed values which breaks consistency with the null branch that throws IllegalArgumentException; update requireLocalDateQuery to catch parsing exceptions (DateTimeParseException or Exception around LocalDate.parse) and rethrow an IllegalArgumentException that includes the parameter name (and wrap the original exception as the cause) so callers always receive an IllegalArgumentException with a clear message identifying the bad param.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt`:
- Around line 55-57: The previous issue (missing exception being passed to the
logger) has been fixed in NotificationDeepLinkParser by using logger.e(e) {
"notification deepLink 파싱에 실패했습니다.: $raw" }; no further code changes are
required—confirm the catch block in NotificationDeepLinkParser.kt still passes
the caught Exception `e` into `logger.e(e) { ... }` to ensure stack traces are
logged and keep the method returning null as currently implemented.
---
Nitpick comments:
In
`@core/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt`:
- Around line 65-67: The helper requireLocalDateQuery currently lets
LocalDate::parse throw DateTimeParseException for malformed values which breaks
consistency with the null branch that throws IllegalArgumentException; update
requireLocalDateQuery to catch parsing exceptions (DateTimeParseException or
Exception around LocalDate.parse) and rethrow an IllegalArgumentException that
includes the parameter name (and wrap the original exception as the cause) so
callers always receive an IllegalArgumentException with a clear message
identifying the bad param.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/main/java/com/yapp/twix/service/TwixFirebaseMessagingService.ktcore/navigation-contract/build.gradle.ktscore/notification/build.gradle.ktscore/notification/src/main/java/com/twix/notification/deeplink/NotificationDeepLinkParser.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- app/src/main/java/com/yapp/twix/service/TwixFirebaseMessagingService.kt
- core/navigation-contract/build.gradle.kts
- core/notification/build.gradle.kts
chanho0908
requested changes
Feb 24, 2026
Member
chanho0908
left a comment
chanho0908
left a comment
There was a problem hiding this comment.
고생했어 현수야 !
내가 FCM을 구현해본적이 없어서 많은 리뷰를 남기진 못했어 🙏
궁금한거 2개만 남겼으니 확인해줘 !
Comment on lines
+13
to
+18
| val coroutineModule = | ||
| module { | ||
| single<CoroutineScope>(named("AppScope")) { | ||
| CoroutineScope(SupervisorJob() + Dispatchers.IO) | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
일전에 DataStore 리뷰할 때 현수가 AuthTokenProvider에 코루틴을 전역으로 주입해서 사용하면
좋겠다고 의견을 줘서 app/di/AppModule 에 named만 지정하지 않은 코루틴 주입 객체가 있는데
혹시 이거랑 다른걸까 ?
| context: Context, | ||
| ) : IdProvider { | ||
| private val dataStore: DataStore<DeviceId> = context.deviceIdDataStore | ||
| private val mutex = Mutex() |
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.4.29.17.mov
리뷰어에게 추가로 요구하는 사항 (선택)