알림 화면 develop 브랜치 반영

[dogmania]

No description provided.

[coderabbitai]

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6234583 and 2589a68.

📒 Files selected for processing (32)

• app/build.gradle.kts
• app/src/main/AndroidManifest.xml
• app/src/main/java/com/yapp/twix/di/FeatureModules.kt
• core/design-system/src/main/res/values/strings.xml
• core/navigation/src/main/java/com/twix/navigation/NavRoutes.kt
• core/network/src/main/java/com/twix/network/model/response/notification/mapper/NotificationMapper.kt
• core/network/src/main/java/com/twix/network/model/response/notification/model/NotificationListResponse.kt
• core/network/src/main/java/com/twix/network/model/response/notification/model/NotificationResponse.kt
• core/network/src/main/java/com/twix/network/service/NotificationService.kt
• data/src/main/java/com/twix/data/repository/DefaultNotificationRepository.kt
• domain/src/main/java/com/twix/domain/model/enums/NotificationType.kt
• domain/src/main/java/com/twix/domain/model/notification/Notification.kt
• domain/src/main/java/com/twix/domain/model/notification/NotificationPage.kt
• domain/src/main/java/com/twix/domain/repository/NotificationRepository.kt
• feature/main/src/main/java/com/twix/home/HomeScreen.kt
• feature/main/src/main/java/com/twix/main/MainScreen.kt
• feature/main/src/main/java/com/twix/main/navigation/MainNavGraph.kt
• feature/notification/.gitignore
• feature/notification/build.gradle.kts
• feature/notification/consumer-rules.pro
• feature/notification/proguard-rules.pro
• feature/notification/src/main/AndroidManifest.xml
• feature/notification/src/main/java/com/twix/notification/NotificationScreen.kt
• feature/notification/src/main/java/com/twix/notification/NotificationViewModel.kt
• feature/notification/src/main/java/com/twix/notification/component/NotificationItem.kt
• feature/notification/src/main/java/com/twix/notification/component/NotificationList.kt
• feature/notification/src/main/java/com/twix/notification/contract/NotificationIntent.kt
• feature/notification/src/main/java/com/twix/notification/contract/NotificationSideEffect.kt
• feature/notification/src/main/java/com/twix/notification/contract/NotificationUiState.kt
• feature/notification/src/main/java/com/twix/notification/di/NotificationFeatureModule.kt
• feature/notification/src/main/java/com/twix/notification/navigation/NotificationNavGraph.kt
• settings.gradle.kts

---

📝 Walkthrough

Walkthrough

새로운 알림(Notification) 기능 모듈을 전체 아키텍처에 통합하는 변경사항입니다. 네트워크 계층에 알림 조회 및 일괄 읽음 처리 API 엔드포인트를 추가하고, 도메인 레이어에 알림 데이터 모델과 저장소 인터페이스를 정의합니다. 데이터 계층에서 저장소 구현체를 작성하고, 새로운 feature/notification 모듈에서 알림 화면, 뷰모델, UI 컴포넌트를 구성합니다. 주요 네비게이션 경로를 추가하고, 메인 화면에서 알림 화면으로의 네비게이션 핸들러를 연결합니다. Koin 기반 의존성 주입 설정을 추가하고, 안드로이드 매니페스트를 업데이트합니다.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

리뷰 포인트 및 개선 제안

긍정적인 측면

✅ 계층화된 아키텍처 준수
네트워크 → 도메인 → 데이터 → 프레젠테이션의 명확한 계층 분리가 잘 유지되어 있으며, 각 계층의 책임이 명확합니다.

✅ 매퍼 패턴 활용
NotificationMapper.kt에서 네트워크 응답을 도메인 모델로 변환하는 확장 함수를 제공하여, 계층 간 데이터 변환을 깔끔하게 처리했습니다.

✅ 타입 안전성
NotificationType 열거형과 fromApi() 함수로 API 응답의 타입 문자열을 안전하게 매핑하고, 알 수 없는 타입에 대해 UNKNOWN으로 폴백하는 방식이 좋습니다.

개선이 필요한 부분

⚠️ ViewModel의 deep link 파싱 로직 복잡도

문제점:
NotificationViewModel.handleNotificationClick() 메서드에서 deep link 파싱과 네비게이션 로직이 혼재되어 있습니다. 조건문이 많고 각 경로별 처리가 복잡합니다.

개선 방향:
deep link 파싱 로직을 별도의 NotificationDeepLinkHandler 또는 DeepLinkNavigator 같은 전용 클래스로 분리하면 더 명확합니다.

// 현재 (복잡함)
private suspend fun handleNotificationClick(notification: Notification) {
    val parsed = notificationDeepLinkParser.parse(notification.deepLink)
    // 10+ 줄의 when 조건문...
}

// 개선 제안
interface DeepLinkNavigator {
    suspend fun navigate(deepLink: String?): NotificationSideEffect?
}

private suspend fun handleNotificationClick(notification: Notification) {
    val sideEffect = deepLinkNavigator.navigate(notification.deepLink)
        ?: NavigateToHome
    emitSideEffect(sideEffect)
}

---

⚠️ fetchNextNotificationList의 중복 제거 로직

문제점:
기존 목록과 새로운 페이지의 알림을 id 기준으로 distinctBy하여 중복을 제거하는데, 매번 모든 데이터를 메모리에 로드합니다. 큰 데이터셋에서는 성능 문제가 발생할 수 있습니다.

검토 질문:

• 실제 사용 시나리오에서 한 번에 로드할 알림의 최대 개수는 예상되십니까?
• 무한 스크롤 구현 시 이전 페이지의 ID를 추적하여 중복 방지를 할 수는 없을까요?

---

⚠️ null 안전성: createdAt 필드

문제점:
NotificationMapper.kt의 String.toLocalDateTimeOrNull() 함수는 파싱 실패 시 null을 반환합니다. 이 경우 Notification의 createdAt이 null이 되는데, UI에서 이를 어떻게 처리하는지 명확하지 않습니다.

개선 방향:

// 현재: null일 수 있음
val createdAt: LocalDateTime?

// 개선: 실패 시 기본값 사용
val createdAt: LocalDateTime = createdAt?.toLocalDateTimeOrNull() 
    ?: LocalDateTime.now()

// 또는: 파싱 실패 로그
private fun String.toLocalDateTimeOrNull(): LocalDateTime? {
    return try {
        OffsetDateTime.parse(this).toLocalDateTime()
    } catch (e: Exception) {
        Log.w("NotificationMapper", "Failed to parse date: $this", e)
        null
    }
}

---

⚠️ markAllNotificationsAsRead의 실패 처리

문제점:
NotificationViewModel의 markAllNotificationAsRead() 메서드는 fire-and-forget 방식으로 실행되어, 실패해도 사용자에게 알려지지 않습니다.

개선 방향:

// 현재 (오류 무시)
private fun markAllNotificationAsRead() {
    viewModelScope.launch {
        notificationRepository.markAllNotificationsAsRead()
    }
}

// 개선: 오류 처리
private fun markAllNotificationAsRead() {
    viewModelScope.launch {
        val result = notificationRepository.markAllNotificationsAsRead()
        if (result is AppResult.Error) {
            emitSideEffect(ShowToast(R.string.toast_mark_as_read_failed, ToastType.ERROR))
        }
    }
}

---

⚠️ NotificationList의 아이템 키 설정

문제점:
NotificationList.kt에서 itemsIndexed를 사용하고 인덱스를 암묵적으로 사용합니다. 목록 순서 변경 시 Compose의 리컴포지션이 비효율적일 수 있습니다.

개선 방향:

// 현재
itemsIndexed(notificationsList) { _, notification ->
    NotificationItem(notification, onClick = onNotificationClick)
}

// 개선: 명시적 key 사용
items(notificationsList, key = { it.id }) { notification ->
    NotificationItem(notification, onClick = onNotificationClick)
}

---

⚠️ NotificationNavGraph의 TODO 주석

문제점:
navigateToStatisticsEndedGoals에서 popBackStack이 임시 구현되어 있으며, "TODO" 주석이 있습니다.

권장사항:
이 부분이 실제로 통계 화면으로 이동해야 한다면, 다음과 같이 명확히 구현하시기 바랍니다:

// 현재
navigateToStatisticsEndedGoals = {
    navController.popBackStack() // TODO: 실제 통계 네비게이션
}

// 개선: 명확한 구현 또는 FIXME 주석
navigateToStatisticsEndedGoals = {
    navController.navigate(NavRoutes.StatisticsRoute.createRoute(goalFilter = "ended")) {
        launchSingleTop = true
    }
}

---

✅ 좋은 점: 문자열 리소스 분리
알림 관련 문자열(word_notification, settings_notification 등)을 리소스로 정의하여 다국어 지원과 유지보수성을 높였습니다.

✅ 좋은 점: State 및 Intent 분리
NotificationUiState, NotificationIntent, NotificationSideEffect를 명확히 분리하여 MVVM 아키텍처가 잘 구현되어 있습니다.

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/#101-notification-screen

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}