Fix #19: Implement complete notification screen with UI, API integration, and tests #22
Conversation
…ionAction, NotificationEvent, NotificationState, and NotificationViewModel
…e NotificationState and ViewModel, and integrate RemoteNotificationDataSource
WalkthroughThis update implements a complete notification screen for a GitHub notifier app, including UI, API integration, data mapping, and error handling. It introduces new presentation models, view models, composables, and utility functions, refactors domain and data layers to support new API requirements, and adds vector icons and unit tests for relative time formatting. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as NotificationScreen
participant VM as NotificationViewModel
participant DS as NotificationDataSource (Remote)
participant API as GitHub API
UI->>VM: Collect state (on composition)
activate VM
VM->>DS: getNotifications(includeRead)
activate DS
DS->>API: GET /notifications?all=includeRead
API-->>DS: List<NotificationDto>
DS-->>VM: Result<List<Notification>>
VM-->>UI: Update state (isLoading=false, notifications)
deactivate DS
deactivate VM
UI->>UI: Render notifications list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
… to show badge for unread notifications
…ccuracy in time representation
…unread notifications
…ime representation
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
app/src/main/java/com/notifier/app/notification/data/util/ZonedDateTimeUtils.kt (1)
19-28: Good nullable receiver update, but consider improving error handling.The change to accept
String?correctly supports the nullable date fields from the API. However, consider using proper logging instead ofprintStackTrace()for production code.Consider replacing
printStackTrace()with proper logging:- e.printStackTrace() + // Log the parsing error appropriately + Log.w("ZonedDateTimeUtils", "Failed to parse date: $this", e)app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt (1)
34-34: Consider usingArrangement.spacedBy()instead ofSpaceEvenly.
Arrangement.SpaceEvenlydistributes available space evenly between all children, which may not be the intended behavior for a notification item layout. Consider usingArrangement.spacedBy(8.dp)or no horizontal arrangement to achieve proper spacing.- horizontalArrangement = Arrangement.SpaceEvenly + horizontalArrangement = Arrangement.spacedBy(8.dp)app/src/main/java/com/notifier/app/notification/presentation/NotificationViewModel.kt (2)
44-44: Consider adding includeRead parameter support.The
getNotifications()call doesn't utilize theincludeReadparameter mentioned in the AI summary. Consider adding this parameter to allow filtering read/unread notifications.- notificationDataSource.getNotifications() + notificationDataSource.getNotifications(includeRead = true)
55-60: Safe error handling with proper type casting.The error handling correctly updates the loading state and casts to
NetworkError. However, consider adding a safe cast or additional error handling for robustness..onError { error -> mutableStateFlow.update { it.copy(isLoading = false) } sendEvent( - NotificationEvent.NetworkErrorEvent(error as NetworkError) + NotificationEvent.NetworkErrorEvent(error as? NetworkError ?: NetworkError.UNKNOWN) ) }app/src/test/java/com/notifier/app/core/presentation/ZonedDateTimeToRelativeStringTest.kt (1)
89-93: Clarify test name to match the actual test case.The test name
testToRelativeTimeString_withNegativeDuration_returns5his misleading since it's testing a positive duration (5 hours in the future), not a negative duration.- fun testToRelativeTimeString_withNegativeDuration_returns5h() { + fun testToRelativeTimeString_with5HoursInFuture_returns5h() {app/src/main/java/com/notifier/app/notification/presentation/model/NotificationUi.kt (1)
62-64: Add input validation for regex matching.The regex pattern could be more robust. Consider validating the input format more thoroughly and handling edge cases.
- val regex = Regex("""https://api.github.com/repos/([^/]+)/([^/]+)/([^/]+)/(.+)""") - val match = regex.find(apiUrl) ?: return repositoryHtmlUrl - val (owner, repo, type, rest) = match.destructured + val regex = Regex("""https://api\.github\.com/repos/([^/]+)/([^/]+)/([^/]+)/(.+)""") + val match = regex.find(apiUrl) ?: return repositoryHtmlUrl + val groups = match.groupValues + if (groups.size < 5) return repositoryHtmlUrl + val (_, owner, repo, type, rest) = groups
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
app/src/main/java/com/notifier/app/core/data/networking/HttpClientFactory.kt(2 hunks)app/src/main/java/com/notifier/app/core/presentation/util/ZonedDateTimeToRelativeString.kt(1 hunks)app/src/main/java/com/notifier/app/di/ApiModule.kt(2 hunks)app/src/main/java/com/notifier/app/notification/data/mappers/NotificationMapper.kt(1 hunks)app/src/main/java/com/notifier/app/notification/data/networking/RemoteNotificationDataSource.kt(2 hunks)app/src/main/java/com/notifier/app/notification/data/networking/dto/NotificationDto.kt(1 hunks)app/src/main/java/com/notifier/app/notification/data/networking/dto/RepositoryDto.kt(0 hunks)app/src/main/java/com/notifier/app/notification/data/networking/dto/SubjectDto.kt(1 hunks)app/src/main/java/com/notifier/app/notification/data/util/ZonedDateTimeUtils.kt(1 hunks)app/src/main/java/com/notifier/app/notification/domain/NotificationDataSource.kt(2 hunks)app/src/main/java/com/notifier/app/notification/domain/model/Notification.kt(1 hunks)app/src/main/java/com/notifier/app/notification/domain/model/Owner.kt(1 hunks)app/src/main/java/com/notifier/app/notification/domain/model/Repository.kt(1 hunks)app/src/main/java/com/notifier/app/notification/domain/model/Subject.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/NotificationAction.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/NotificationEvent.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/NotificationRoute.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/NotificationScreen.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/NotificationState.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/NotificationViewModel.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt(1 hunks)app/src/main/java/com/notifier/app/notification/presentation/model/NotificationUi.kt(1 hunks)app/src/main/res/drawable/ic_discussion.xml(1 hunks)app/src/main/res/drawable/ic_issue.xml(1 hunks)app/src/main/res/drawable/ic_pull.xml(1 hunks)app/src/test/java/com/notifier/app/core/presentation/ZonedDateTimeToRelativeStringTest.kt(1 hunks)
💤 Files with no reviewable changes (1)
- app/src/main/java/com/notifier/app/notification/data/networking/dto/RepositoryDto.kt
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: theMr17
PR: theMr17/github-notifier#6
File: gradle/libs.versions.toml:2-10
Timestamp: 2025-03-31T15:19:22.452Z
Learning: The GitHub Notifier project uses up-to-date library versions as of March 2025, including AGP 8.9.1, Kotlin 2.1.10, Core KTX 1.15.0, and other dependencies with 2025-dated versions. These versions are valid despite appearing to be from the future from perspectives prior to 2025.
Learnt from: theMr17
PR: theMr17/github-notifier#23
File: app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt:37-53
Timestamp: 2025-06-14T12:33:29.399Z
Learning: The github-notifier Android app has minSdkVersion set to 26, so API-level guards for Android 8.0+ features like NotificationChannel are not required.
Learnt from: theMr17
PR: theMr17/github-notifier#6
File: gradle/libs.versions.toml:2-10
Timestamp: 2025-03-31T15:19:22.452Z
Learning: The user prefers to use future-dated version numbers in dependency declarations for the GitHub Notifier project, despite these versions not being officially released.
app/src/main/java/com/notifier/app/notification/domain/model/Repository.kt (1)
Learnt from: theMr17
PR: theMr17/github-notifier#23
File: app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt:37-53
Timestamp: 2025-06-14T12:33:29.399Z
Learning: The github-notifier Android app has minSdkVersion set to 26, so API-level guards for Android 8.0+ features like NotificationChannel are not required.
app/src/main/java/com/notifier/app/notification/domain/model/Notification.kt (2)
Learnt from: theMr17
PR: theMr17/github-notifier#23
File: app/src/main/java/com/notifier/app/core/domain/notification/NotificationHandler.kt:37-53
Timestamp: 2025-06-14T12:33:29.399Z
Learning: The github-notifier Android app has minSdkVersion set to 26, so API-level guards for Android 8.0+ features like NotificationChannel are not required.
Learnt from: theMr17
PR: theMr17/github-notifier#6
File: gradle/libs.versions.toml:2-10
Timestamp: 2025-03-31T15:19:22.452Z
Learning: The GitHub Notifier project uses up-to-date library versions as of March 2025, including AGP 8.9.1, Kotlin 2.1.10, Core KTX 1.15.0, and other dependencies with 2025-dated versions. These versions are valid despite appearing to be from the future from perspectives prior to 2025.
🧬 Code Graph Analysis (3)
app/src/main/java/com/notifier/app/notification/data/networking/RemoteNotificationDataSource.kt (2)
app/src/main/java/com/notifier/app/core/data/networking/safeCall.kt (1)
safeCall(22-37)app/src/main/java/com/notifier/app/core/data/networking/constructUrl.kt (1)
constructUrl(14-25)
app/src/main/java/com/notifier/app/notification/presentation/NotificationRoute.kt (2)
app/src/main/java/com/notifier/app/core/presentation/util/ObserveAsEvents.kt (1)
ObserveAsEvents(12-27)app/src/main/java/com/notifier/app/notification/presentation/NotificationScreen.kt (1)
NotificationScreen(22-45)
app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt (1)
app/src/main/java/com/notifier/app/ui/theme/Theme.kt (1)
GitHubNotifierTheme(39-61)
🔇 Additional comments (32)
app/src/main/res/drawable/ic_discussion.xml (1)
1-10: LGTM! Well-structured vector drawable following Android conventions.The vector drawable is properly formatted with standard Android attributes, appropriate dimensions (24dp), and uses GitHub's brand green color. The path data correctly defines a discussion/chat bubble icon that will integrate well with the notification UI.
app/src/main/java/com/notifier/app/core/presentation/util/ZonedDateTimeToRelativeString.kt (1)
7-26: LGTM! Clean and efficient relative time formatting implementation.The extension function correctly calculates time differences using
Duration.between()and converts to appropriate units. The use ofabs()ensures it handles both past and future times consistently. The cascading unit logic (seconds → minutes → hours → days → weeks) is appropriate for UI display.app/src/main/java/com/notifier/app/notification/domain/model/Repository.kt (1)
1-1: LGTM! Package refactoring improves code organization.Moving domain models to the
.modelsubpackage follows good architectural practices and improves code organization. The change is consistent with the broader domain model refactoring.app/src/main/java/com/notifier/app/notification/domain/model/Notification.kt (1)
1-1: LGTM! Consistent package refactoring with other domain models.The package move to
.modelsubpackage is consistent with the broader domain model reorganization and follows good architectural practices. The Notification data class structure remains appropriate for GitHub API integration.app/src/main/java/com/notifier/app/notification/domain/model/Owner.kt (1)
1-1: Package restructuring approved and imports verifiedAll references to the domain model
Ownernow correctly import fromcom.notifier.app.notification.domain.model.Owner, and theOwnerDtoimport remains appropriately scoped to the DTO package inNotificationMapper.kt. No further changes needed.app/src/main/java/com/notifier/app/notification/data/mappers/NotificationMapper.kt (1)
8-11: Import updates are consistent with domain model restructuring.All domain model imports have been correctly updated to reference the new
.modelsubpackage. The mapper logic remains unchanged, which is appropriate for this refactoring.app/src/main/java/com/notifier/app/notification/data/networking/dto/SubjectDto.kt (1)
9-9: Approve nullablelatestCommentUrlupdateThe
latestCommentUrlproperty is now nullable in both the DTO and the domain model, and the mapper passes it through correctly:
- app/src/main/java/com/notifier/app/notification/domain/model/Subject.kt:
val latestCommentUrl: String?- app/src/main/java/com/notifier/app/notification/data/mappers/NotificationMapper.kt:
latestCommentUrl = latestCommentUrlNo further changes needed.
app/src/main/res/drawable/ic_pull.xml (1)
1-11: Well-designed vector drawable for pull request notifications.The icon follows Android best practices with appropriate dimensions, semantic green color, and a clear git branch merge design that's fitting for pull request notifications.
app/src/main/java/com/notifier/app/notification/data/networking/dto/NotificationDto.kt (1)
11-11: Nullable lastReadAt handled correctlyThe DTO’s
lastReadAtis now nullable, the mapper in NotificationMapper.kt usesString?.toZonedDateTimeOrDefault()to safely produce a non‐null ZonedDateTime, and the extension in ZonedDateTimeUtils.kt accepts null receivers and falls back to the default. No further changes required.app/src/main/res/drawable/ic_issue.xml (1)
1-15: Well-structured vector drawable for issue notifications.The icon follows Android vector drawable best practices with appropriate dimensions, color scheme, and path structure. The green color (#FF1A7F37) aligns with GitHub's visual language for issues.
app/src/main/java/com/notifier/app/di/ApiModule.kt (2)
6-7: Proper imports for notification data source components.The imports are correctly added to support the new notification data source provider.
35-41: Well-implemented dependency injection for notification data source.The provider method follows established patterns with proper annotations, dependency injection, and interface-based return type. The singleton scope is appropriate for a data source.
app/src/main/java/com/notifier/app/notification/domain/model/Subject.kt (2)
1-1: Good package organization improvement.Moving domain models to a dedicated
.modelsubpackage improves code organization and follows common architectural patterns.
4-4: Appropriate nullable type for optional API field.Making
latestCommentUrlnullable accurately reflects that not all GitHub notifications may have a latest comment URL, improving data model accuracy.app/src/main/java/com/notifier/app/notification/presentation/NotificationState.kt (1)
1-10: Well-designed immutable state class for Compose.The state class follows Compose best practices with proper
@Immutableannotation, sensible default values, and clean structure. It effectively encapsulates the UI state for notifications.app/src/main/java/com/notifier/app/notification/presentation/NotificationRoute.kt (2)
4-7: Proper imports for modern Compose state management.The imports correctly include the necessary components for lifecycle-aware state collection and event observation.
14-26: Well-implemented Compose route with proper state management.The route correctly uses
hiltViewModel()for dependency injection,collectAsStateWithLifecycle()for state collection, andObserveAsEventsfor event handling. The empty event handler is acceptable and allows for future event implementation.app/src/main/java/com/notifier/app/notification/presentation/NotificationAction.kt (1)
1-7: Well-structured action pattern implementation.The sealed interface approach provides type-safe action handling and follows clean architecture principles. The single action
OnNotificationItemClickappropriately encapsulates the notification data needed for handling item clicks.app/src/main/java/com/notifier/app/notification/domain/NotificationDataSource.kt (2)
5-5: Good import organization.The addition of the Notification domain model import aligns with the updated method signature and follows proper dependency management.
23-28: Excellent API design with backward compatibility.The addition of the optional
includeReadparameter with a default value oftruemaintains backward compatibility while extending functionality. The parameter naming is clear and the documentation thoroughly explains the behavior.app/src/main/java/com/notifier/app/notification/presentation/NotificationEvent.kt (1)
6-9: Clean event modeling for error handling.The sealed interface approach with specific error event types provides type-safe error handling in the presentation layer. The separation between
NetworkErrorEventandPersistenceErrorEventallows for granular error handling strategies.app/src/main/java/com/notifier/app/notification/data/networking/RemoteNotificationDataSource.kt (2)
9-11: Appropriate import updates.The imports correctly reflect the updated response structure, changing from wrapped response DTO to direct list of notification DTOs, and adding the domain model import.
37-49: Well-implemented API integration with improved RESTful design.The implementation correctly:
- Updates the method signature to match the domain interface
- Changes to plural "/notifications" endpoint (more RESTful)
- Adds query parameter for filtering read notifications
- Handles the simplified response structure (direct list instead of wrapped)
- Maintains proper error handling with
safeCall- Uses established URL construction patterns
The mapping logic correctly transforms the DTO list to domain objects.
app/src/main/java/com/notifier/app/core/data/networking/HttpClientFactory.kt (1)
14-14: Good import addition for header functionality.The import correctly supports the updated header setting approach.
app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt (1)
82-93: LGTM! Well-structured preview data.The preview function provides comprehensive sample data that effectively demonstrates the component's appearance and behavior.
app/src/main/java/com/notifier/app/notification/presentation/NotificationViewModel.kt (2)
23-32: LGTM! Proper StateFlow implementation with lifecycle awareness.The StateFlow setup correctly uses
onStartto trigger initial loading andWhileSubscribedwith a reasonable timeout for efficient lifecycle management.
34-40: Action handler is ready for implementation.The action handling structure is properly set up for future notification item click handling.
app/src/test/java/com/notifier/app/core/presentation/ZonedDateTimeToRelativeStringTest.kt (1)
8-100: Excellent test coverage with comprehensive boundary testing.The test suite thoroughly covers all time units and edge cases, including:
- Boundary conditions between time units
- Both past and future times
- Edge cases like same time (0s)
- Large durations (365 days, 1000 days)
This provides robust validation for the relative time formatting functionality.
app/src/main/java/com/notifier/app/notification/presentation/model/NotificationUi.kt (2)
29-42: LGTM! Comprehensive reason mapping with fallback.The notification reason mapping covers a wide range of GitHub notification types and provides meaningful descriptions for users. The fallback case ensures no reason is left unmapped.
66-74: Good URL mapping with comprehensive type coverage.The URL mapping correctly handles various GitHub content types and provides appropriate fallbacks for unrecognized types.
app/src/main/java/com/notifier/app/notification/presentation/NotificationScreen.kt (2)
27-35: LGTM! Clean loading state handling.The early return pattern for loading state with centered progress indicator provides good UX.
47-80: Well-structured preview parameter provider.The preview parameter provider effectively demonstrates both loading and loaded states with randomized data that covers different notification types.
app/src/main/java/com/notifier/app/core/data/networking/HttpClientFactory.kt
Show resolved
Hide resolved
app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt
Show resolved
Hide resolved
app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt
Show resolved
Hide resolved
app/src/main/java/com/notifier/app/notification/presentation/components/NotificationItem.kt
Show resolved
Hide resolved
app/src/main/java/com/notifier/app/notification/presentation/model/NotificationUi.kt
Show resolved
Hide resolved
app/src/main/java/com/notifier/app/notification/presentation/NotificationScreen.kt
Outdated
Show resolved
Hide resolved
…tter clarity and maintainability
…tation for improved clarity and maintainability
…across multiple files
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Refactor
Documentation
Issue Reference
Screenshots
TODO: Add screenshots
Essential Checklist