Feature/base project#11
Conversation
There was a problem hiding this comment.
Pull request overview
This PR appears to modernize the project’s build/tooling setup and introduce a matches UI flow with notification scheduling support.
Changes:
- Upgraded build tooling (Gradle wrapper, AGP/Kotlin/Compose plugin versions) and adjusted module namespaces/SDK/JVM targets.
- Added a Compose-based main UI (Activity/Screen/ViewModel) that fetches matches and toggles notifications.
- Introduced domain use cases and a WorkManager
Workerentrypoint to schedule/cancel match notifications.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle | Minor project settings tweak (whitespace). |
| notification-scheduler/src/main/kotlin/me/dio/copa/catar/notification/scheduler/extensions/NotificationMatcherWorker.kt | Adds WorkManager worker to schedule/cancel match notifications. |
| notification-scheduler/build.gradle | Adds module namespace and adjusts Android config blocks. |
| gradle/wrapper/gradle-wrapper.properties | Upgrades Gradle wrapper distribution and adds SHA sum. |
| gradle.properties | Updates Gradle/Android build properties and feature flags. |
| domain/src/main/kotlin/me/dio/copa/catar/domain/usecase/GetMatchesUseCase.kt | Adds use case to fetch matches. |
| domain/src/main/kotlin/me/dio/copa/catar/domain/usecase/EnableNotificationUseCase.kt | Adds use case to enable notifications for a match. |
| domain/src/main/kotlin/me/dio/copa/catar/domain/usecase/DesableNotificationUseCase.kt | Adds use case to disable notifications for a match. |
| domain/build.gradle | Adjusts Kotlin compilation options (JVM target). |
| data/remote/build.gradle | Updates JVM compatibility/toolchain for the remote module. |
| data/local/src/main/kotlin/me/dio/copa/catar/local/di/LocalModule.kt | Refactors local DI module bindings/providers. |
| data/local/src/main/AndroidManifest.xml | Removes manifest package in favor of namespace. |
| data/local/build.gradle | Adds module namespace and adjusts Android/dependency config. |
| data/data/build.gradle | Adjusts Kotlin compilation options (JVM target). |
| build.gradle | Upgrades AGP/Kotlin/Hilt plugin versions and Compose plugin wiring. |
| app/src/main/kotlin/me/dio/copa/catar/features/MainViewModel.kt | Adds ViewModel for loading matches and toggling notifications. |
| app/src/main/kotlin/me/dio/copa/catar/features/MainScreen.kt | Adds Compose UI for rendering matches and notification toggle control. |
| app/src/main/kotlin/me/dio/copa/catar/features/MainActivity.kt | Adds new Hilt-enabled Activity hosting the Compose UI. |
| app/src/main/kotlin/me/dio/copa/catar/core/BaseViewModel.kt | Minor formatting/whitespace adjustment. |
| app/src/main/kotlin/me/dio/copa/catar/MainActivity.kt | Removes the previous placeholder Activity. |
| app/src/main/AndroidManifest.xml | Points launcher activity to the new features.MainActivity. |
| app/build.gradle | Updates Android/Compose configuration, SDK/JVM levels, and dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import android.R.attr.contentDescription | ||
| import android.R.attr.onClick |
There was a problem hiding this comment.
Unused imports (android.R.attr.contentDescription, android.R.attr.onClick) should be removed to keep the file clean and avoid confusion with Compose parameters.
| import android.R.attr.contentDescription | |
| import android.R.attr.onClick |
| val title = inputData.getString("NOTIFICATION_TITLE_KEY") ?: throw IllegalArgumentException("Title is required.") | ||
| val content = inputData.getString("NOTIFICATION_CONTENT_KEY") ?: throw IllegalArgumentException("Content is required.") | ||
|
|
||
| context.showNotification(title, content) |
There was a problem hiding this comment.
In doWork(), the notification data is read with hardcoded keys and missing values throw IllegalArgumentException, which will crash the worker execution path. Prefer using the NOTIFICATION_*_KEY constants for both writing/reading, and return Result.failure() (optionally with outputData) when required input is absent instead of throwing.
| val title = inputData.getString("NOTIFICATION_TITLE_KEY") ?: throw IllegalArgumentException("Title is required.") | |
| val content = inputData.getString("NOTIFICATION_CONTENT_KEY") ?: throw IllegalArgumentException("Content is required.") | |
| context.showNotification(title, content) | |
| val title = inputData.getString(NOTIFICATION_TITLE_KEY) | |
| val content = inputData.getString(NOTIFICATION_CONTENT_KEY) | |
| if (title == null || content == null) { | |
| return Result.failure() | |
| } | |
| context.showNotification(title, content) |
| is MainUiAction.MatchesNotFound -> TODO() | ||
| MainUiAction.Unexpected -> TODO() |
There was a problem hiding this comment.
Both TODO() branches in observeActions() will crash the app if these actions are emitted (and MatchesNotFound/Unexpected can be triggered by the ViewModel). Replace these with real UI handling (e.g., snackbar/dialog/state) or at least no-op/logging to avoid runtime crashes.
| is MainUiAction.MatchesNotFound -> TODO() | |
| MainUiAction.Unexpected -> TODO() | |
| is MainUiAction.MatchesNotFound -> { | |
| Log.e("MainActivity", "No matches found for the requested criteria.") | |
| } | |
| MainUiAction.Unexpected -> { | |
| Log.e("MainActivity", "An unexpected error occurred while processing UI action.") | |
| } |
| .flowOn(Dispatchers.Main) | ||
| .catch{ | ||
| when(it){ | ||
| is NotFoundException -> | ||
| sendAction(MainUiAction.MatchesNotFound(it.message ?: "Erro sem mensagem")) | ||
| is UnexpectedException -> | ||
| sendAction(MainUiAction.Unexpected) | ||
| } | ||
| } | ||
| .collect { matches -> | ||
| setState { | ||
| copy(matches = matches) | ||
| } | ||
| } |
There was a problem hiding this comment.
flowOn(Dispatchers.Main) shifts upstream work (including the remote fetch + mapping in MatchesRepositoryImpl.getMatches()) onto the main thread. This is the opposite of what you typically want; use an IO/Default dispatcher for upstream work or move the fetch to a background context.
Also, the catch { when(it) { ... } } block has no else/rethrow, so any unexpected exception type will be silently swallowed and the flow will complete without reporting an error.
| .flowOn(Dispatchers.Main) | |
| .catch{ | |
| when(it){ | |
| is NotFoundException -> | |
| sendAction(MainUiAction.MatchesNotFound(it.message ?: "Erro sem mensagem")) | |
| is UnexpectedException -> | |
| sendAction(MainUiAction.Unexpected) | |
| } | |
| } | |
| .collect { matches -> | |
| setState { | |
| copy(matches = matches) | |
| } | |
| } | |
| .flowOn(Dispatchers.IO) | |
| .catch { throwable -> | |
| when (throwable) { | |
| is NotFoundException -> | |
| sendAction(MainUiAction.MatchesNotFound(throwable.message ?: "Erro sem mensagem")) | |
| is UnexpectedException -> | |
| sendAction(MainUiAction.Unexpected) | |
| else -> throw throwable | |
| } | |
| } | |
| .collect { matches -> | |
| setState { | |
| copy(matches = matches) | |
| } |
|
|
||
| runCatching { | ||
| withContext(Dispatchers.Main){ | ||
| val action = if (match.notificationEnabled) { | ||
| disableNotificationUseCase(match.id) | ||
| MainUiAction.DisableNotification(match) | ||
| } else{ | ||
| enableNotificationUseCase(match.id) | ||
| MainUiAction.EnableNotification(match) | ||
| } | ||
|
|
||
| sendAction(action) | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
runCatching { ... } is currently ignoring failures (no onFailure/rethrow), so exceptions during enable/disable will be silently swallowed and the UI won’t be notified. Also withContext(Dispatchers.Main) forces the DataStore write (enable/disableNotificationUseCase) onto the main thread; prefer running the repository call off-main and only posting the resulting UI action back on main.
| runCatching { | |
| withContext(Dispatchers.Main){ | |
| val action = if (match.notificationEnabled) { | |
| disableNotificationUseCase(match.id) | |
| MainUiAction.DisableNotification(match) | |
| } else{ | |
| enableNotificationUseCase(match.id) | |
| MainUiAction.EnableNotification(match) | |
| } | |
| sendAction(action) | |
| } | |
| } | |
| val result = runCatching { | |
| // Execute use cases off the main thread | |
| withContext(Dispatchers.IO) { | |
| if (match.notificationEnabled) { | |
| disableNotificationUseCase(match.id) | |
| MainUiAction.DisableNotification(match) | |
| } else { | |
| enableNotificationUseCase(match.id) | |
| MainUiAction.EnableNotification(match) | |
| } | |
| } | |
| } | |
| result | |
| .onSuccess { action -> | |
| // Post UI update back to main thread | |
| withContext(Dispatchers.Main) { | |
| sendAction(action) | |
| } | |
| } | |
| .onFailure { | |
| // Notify UI about the unexpected error | |
| withContext(Dispatchers.Main) { | |
| sendAction(MainUiAction.Unexpected) | |
| } | |
| } |
| val initialDelay = Duration.between(LocalDateTime.now(), matchDate).minusMinutes(15) | ||
| val inputData = workDataOf( |
There was a problem hiding this comment.
initialDelay can become negative when matchDate is in the past or within the 15-minute window. WorkManager requires a non-negative initial delay; a negative value can throw at runtime when building/enqueuing the request. Consider clamping to zero (or skipping scheduling) when initialDelay.isNegative / initialDelay.isZero.
| private fun createRequest(initialDelay: Duration, inputData: Data): OneTimeWorkRequest = | ||
| OneTimeWorkRequestBuilder<NotificationMatcherWorker>() | ||
| .setInitialDelay(initialDelay) | ||
| .setInputData(inputData) |
There was a problem hiding this comment.
setInitialDelay(initialDelay) is passing a java.time.Duration. This won’t compile against the current dependency (androidx.work:work-runtime-ktx:2.7.1 in this module) which only provides the setInitialDelay(long, TimeUnit) overload. Either update WorkManager to a version that supports Duration, or convert the duration to millis and pass an explicit TimeUnit.
| } | ||
|
|
||
| android { | ||
| namespace 'me.dio.copa.catar.data.local' |
There was a problem hiding this comment.
Changing the module namespace will change the androidTest package name. There is an existing instrumentation test asserting "me.dio.copa.catar.local.test" which will now fail (likely becoming me.dio.copa.catar.data.local.test). Update/remove that assertion to match the new namespace-derived test package.
| namespace 'me.dio.copa.catar.data.local' | |
| namespace 'me.dio.copa.catar.data.local' | |
| testNamespace 'me.dio.copa.catar.local.test' |
| implementation 'androidx.lifecycle:lifecycle-viewmodel-android:2.10.0' | ||
| implementation 'com.google.dagger:hilt-android:2.48' |
There was a problem hiding this comment.
This module adds hilt-android:2.48 while still using dagger/dagger-compiler:2.44. Mixing Dagger/Hilt versions is a common source of kapt/classpath issues; align Dagger artifacts to the same version as Hilt (or remove the Hilt dependency if this module doesn’t use Hilt). Also, lifecycle-viewmodel-android and hilt-android appear unused in this module’s sources—consider removing to keep the dependency graph minimal.
| implementation 'androidx.lifecycle:lifecycle-viewmodel-android:2.10.0' | |
| implementation 'com.google.dagger:hilt-android:2.48' |
| } | ||
| } | ||
|
|
||
| fun toogleNotification(match: Match) { |
There was a problem hiding this comment.
Spelling: toogleNotification should be toggleNotification for clarity and consistency.
No description provided.