🔀 :: (#797) build flavors 적용#801
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds dev/prod product flavors with flavor-specific source sets and dependency routing; introduces dev-only app wiring (DevApplication, dev MainActivity, messaging service, UI, ViewModel) and prod-only Hilt DI modules (network, serialization, app config); enables serialization Gradle plugin. Changes
Sequence Diagram(s)sequenceDiagram
participant Activity as MainActivity
participant PlayCore as Play Core (AppUpdateManager)
participant ViewModel as MainActivityViewModel
Activity->>PlayCore: requestAppUpdateInfo()
PlayCore-->>Activity: updateAvailability(result)
alt update available
Activity->>PlayCore: startImmediateUpdate(intentSender)
PlayCore-->>Activity: activityResult(success/failure)
alt failure
Activity->>ViewModel: onUpdateFailed()
end
else no update
Activity-->>Activity: no-op (early return)
end
sequenceDiagram
participant FCM as Firebase
participant Service as DmsMessagingService
participant TokenMgr as DeviceTokenManager
participant Notif as NotificationManager
FCM->>Service: onNewToken(token)
Service->>TokenMgr: saveDeviceToken(token) (IO)
FCM->>Service: onMessageReceived(remotemsg)
Service->>Notif: build/send notification(title, body)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/build.gradle.kts (1)
66-71: DuplicateaaptOptions.cruncherEnabled = falsesetting.Line 68 and line 70 both set
aaptOptions.cruncherEnabled = false. Remove the duplicate.debug { splits.abi.isEnable = false aaptOptions.cruncherEnabled = false splits.density.isEnable = false - aaptOptions.cruncherEnabled = false }
🧹 Nitpick comments (6)
app/src/dev/kotlin/team/aliens/dms/android/app/EdgeToEdgeConfig.kt (1)
1-1: Align package name with directory structure (or vice versa).The file lives under
team/aliens/dms/android/appbut the package isteam.aliens.dms.android. Consider either updating the package toteam.aliens.dms.android.appor moving the file so directory and package stay in sync for easier navigation and refactoring.app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (2)
25-27: Unused parameter:windowSizeClassThe
windowSizeClassparameter is declared but not used in the function body. Consider either using it for responsive layouts or removing it if not needed for this dev flavor scaffold.
45-48: Consider using the design systemButtonfor consistency.This uses
androidx.compose.material3.Button(line 6) rather than the customButtonfromteam.aliens.dms.android.core.designsystem.Buttons. Since this is a dev flavor scaffold for experimentation, this may be intentional, but if consistency with the design system is desired, consider switching to the design system component.-import androidx.compose.material3.Button +import team.aliens.dms.android.core.designsystem.ContainedButton- Button(onClick = { backStack.add(ScreenB) }) { - Text("Go to Screen B") - } + ContainedButton(onClick = { backStack.add(ScreenB) }) { + Text("Go to Screen B") + }app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt (3)
34-40: Complete the update cancellation/failure handling.The TODO comment indicates that forced update modal handling is not yet implemented. When users cancel or fail the update, the app should handle this appropriately.
Do you want me to generate an implementation for the forced update modal or open a new issue to track this task?
46-49: Consider replacing deprecated FLAG_LAYOUT_NO_LIMITS.The
FLAG_LAYOUT_NO_LIMITSflag is deprecated and may cause issues on newer Android versions (especially with display cutouts and notches). Consider usingWindowInsetsControllerwithsystemBarsBehaviorinstead for better compatibility.Apply this diff to modernize the window configuration:
- window.setFlags( - WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS, - WindowManager.LayoutParams.FLAG_LAYOUT_NO_LIMITS, - ) + WindowCompat.getInsetsController(window, window.decorView).apply { + systemBarsBehavior = WindowInsetsControllerCompat.BEHAVIOR_SHOW_TRANSIENT_BARS_BY_SWIPE + }Note: Add the import
androidx.core.view.WindowInsetsControllerCompatif you apply this change.
54-54: Complete adaptive layout implementation.The TODO comment indicates that adaptive layout support is pending. This is an important feature for better tablet and foldable device support.
Do you want me to help implement adaptive layout handling based on the
windowSizeClassor open a new issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/build.gradle.kts(4 hunks)app/src/dev/AndroidManifest.xml(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/DevApplication.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/EdgeToEdgeConfig.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/service/DmsMessagingService.kt(1 hunks)app/src/prod/AndroidManifest.xml(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/di/app/AppConfigModule.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/di/network/NetworkConfigModule.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/di/serialization/SerializationModule.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/navigation/DmsNavigator.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavGraph.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavigator.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (1)
Button(42-92)
app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt (2)
app/src/prod/kotlin/team/aliens/dms/android/app/MainActivity.kt (1)
checkAppUpdate(68-84)app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
DmsApp(25-59)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: build
🔇 Additional comments (16)
app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavigator.kt (1)
14-14: LGTM for prod flavor navigation setup.The Volunteers feature is properly wired into the authorized navigation interface. The import and interface inheritance are correctly positioned and follow the established patterns.
Note: Ensure a corresponding dev flavor variant exists at
app/src/dev/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavigator.ktwith appropriate feature set for the dev environment.Also applies to: 28-28
app/src/prod/kotlin/team/aliens/dms/android/app/navigation/DmsNavigator.kt (1)
48-48: Import correctly enables existing openVolunteer() implementation.The added import on Line 48 properly backs the
openVolunteer()method at Line 104, which was already present. This import addition resolves a missing dependency.app/src/prod/kotlin/team/aliens/dms/android/app/navigation/authorized/AuthorizedNavGraph.kt (1)
21-21: Navigation graph correctly incorporates Volunteers feature.The nested nav graph and destination screen additions follow the established patterns and properly integrate the Volunteers feature into the authorized navigation structure. Internal consistency verified across imports and graph composition.
Also applies to: 26-26, 37-37, 56-56
app/src/dev/kotlin/team/aliens/dms/android/app/EdgeToEdgeConfig.kt (1)
7-13: Edge‑to‑edge setup and API guard look good.The
ComponentActivityextension cleanly centralizes edge‑to‑edge setup, and theSDK_INT >= Qguard aroundisNavigationBarContrastEnforcedis correct and avoids API‑level crashes.app/src/prod/kotlin/team/aliens/dms/android/app/di/network/NetworkConfigModule.kt (5)
1-24: LGTM!The module setup is correct with proper Hilt annotations and well-organized imports. The
BuildConfigalias is a good practice to avoid ambiguity in multi-module projects.
26-40: LGTM!URL providers are correctly implemented with proper singleton scoping and qualifier annotations.
117-125: LGTM!Default interceptors provider is correctly implemented.
127-135: LGTM!Global interceptors provider is correctly implemented with the JWT interceptor for authenticated request handling.
74-103: > Likely an incorrect or invalid review comment.app/src/prod/kotlin/team/aliens/dms/android/app/di/serialization/SerializationModule.kt (1)
11-13: LGTM!The module is correctly configured as a Hilt singleton component module.
app/build.gradle.kts (2)
30-55: LGTM! Well-structured flavor configuration.The environment-based flavor setup correctly separates dev and prod configurations with appropriate source sets, application ID suffix, and version name suffix for dev builds. The BuildConfig field
ENVIRONMENTprovides runtime differentiation.
122-131: Different navigation systems per flavor - verify this is intentional.
- dev uses
navigation3.runtimeandnavigation3.ui(experimental Navigation 3)- prod uses
navigation.composeandhilt.navigation.compose(stable Navigation Compose) with Compose DestinationsThis divergence means dev and prod have fundamentally different navigation architectures. Ensure this is intentional for prototyping Navigation 3 in dev while maintaining stable navigation in prod.
app/src/prod/AndroidManifest.xml (1)
1-8: LGTM! The manifest correctly usestools:replace="android:name"to override the application class for the prod flavor.DmsApplicationis properly defined in the prod source set.app/src/prod/kotlin/team/aliens/dms/android/app/di/app/AppConfigModule.kt (1)
10-17: LGTM. The module correctly provides a singletonZoneOffset.UTCfor consistent timezone handling. Both prod and dev flavors have identicalAppConfigModuleimplementations, so there are no flavor-specific inconsistencies.app/src/dev/kotlin/team/aliens/dms/android/app/DevApplication.kt (1)
9-19: LGTM! Standard Hilt + WorkManager integration.The implementation correctly:
- Uses
@HiltAndroidAppfor Hilt component generation- Implements
Configuration.Providerfor custom WorkManager configuration- Injects
HiltWorkerFactoryfor DI-enabled WorkersThe prod
DmsApplicationhas equivalent WorkManager setup with identical implementation, and Workers are used in production (MealWorker in the core/widget module).app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
12-15: Navigation3 library is now at release-candidate status, not alpha.
androidx.navigation3APIs have progressed to version 1.0.0-rc01 (as of November 2025) and are no longer experimental or alpha. For the dev flavor, this RC dependency is acceptable, though the library has not yet reached a final 1.0.0 stable release.
| lifecycleScope.launch { | ||
| deviceTokenManager.fetchDeviceToken() | ||
| } |
There was a problem hiding this comment.
Add error handling for device token fetching.
The device token fetch operation has no error handling. If fetchDeviceToken() throws an exception, it could crash the app or fail silently. Consider adding try-catch or collecting the result to handle failures appropriately.
Apply this diff to add error handling:
lifecycleScope.launch {
- deviceTokenManager.fetchDeviceToken()
+ runCatching {
+ deviceTokenManager.fetchDeviceToken()
+ }.onFailure { exception ->
+ // TODO: Log error or show user notification
+ }
}📝 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.
| lifecycleScope.launch { | |
| deviceTokenManager.fetchDeviceToken() | |
| } | |
| lifecycleScope.launch { | |
| runCatching { | |
| deviceTokenManager.fetchDeviceToken() | |
| }.onFailure { exception -> | |
| // TODO: Log error or show user notification | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt around lines
63 to 65, the coroutine calling deviceTokenManager.fetchDeviceToken() lacks
error handling; wrap the call in a try-catch inside the lifecycleScope.launch or
handle the result returned by fetchDeviceToken(), catching exceptions and
logging them and/or reporting to your error telemetry, and optionally retrying
or surfacing a user-friendly fallback; ensure exceptions are not allowed to
propagate and crash the app.
| private val notificationManager: NotificationManager by lazy { | ||
| NotificationManager(context = this) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Inject NotificationManager instead of manual instantiation.
The NotificationManager is manually instantiated with lazy, while DeviceTokenManager is properly injected. This breaks the DI pattern and makes the service harder to test and maintain.
Apply this diff to inject NotificationManager:
+ @Inject
+ lateinit var notificationManager: NotificationManager
+
- private val notificationManager: NotificationManager by lazy {
- NotificationManager(context = this)
- }Ensure that NotificationManager has a Hilt binding configured in your DI modules.
🤖 Prompt for AI Agents
In app/src/dev/kotlin/team/aliens/dms/android/app/service/DmsMessagingService.kt
around lines 19 to 21, the NotificationManager is being manually instantiated
with a lazy delegate which breaks DI and testability; replace the lazy/manual
instantiation with a Hilt-injected property (e.g., add an @Inject lateinit var
notificationManager: NotificationManager to the service), remove the lazy block,
ensure the service is annotated with @AndroidEntryPoint (or otherwise
participates in Hilt), and confirm NotificationManager is provided in your Hilt
module/bindings so Hilt can inject it.
| CoroutineScope(Dispatchers.IO).launch { | ||
| deviceTokenManager.saveDeviceToken(deviceToken = deviceToken) | ||
| } |
There was a problem hiding this comment.
Use structured concurrency or goAsync() to ensure token persistence.
Creating an unmanaged CoroutineScope(Dispatchers.IO) is risky because the coroutine may be cancelled if the service is destroyed before saveDeviceToken completes, resulting in the token not being saved.
Apply this diff to ensure the work completes using goAsync():
override fun onNewToken(deviceToken: String) {
super.onNewToken(deviceToken)
- CoroutineScope(Dispatchers.IO).launch {
+ val pendingResult = goAsync()
+ CoroutineScope(Dispatchers.IO).launch {
- deviceTokenManager.saveDeviceToken(deviceToken = deviceToken)
+ try {
+ deviceTokenManager.saveDeviceToken(deviceToken = deviceToken)
+ } catch (e: Exception) {
+ // Log error
+ } finally {
+ pendingResult.finish()
+ }
}
}The goAsync() method extends the service's lifetime, ensuring the coroutine can complete even if the service is being destroyed.
📝 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.
| CoroutineScope(Dispatchers.IO).launch { | |
| deviceTokenManager.saveDeviceToken(deviceToken = deviceToken) | |
| } | |
| val pendingResult = goAsync() | |
| CoroutineScope(Dispatchers.IO).launch { | |
| try { | |
| deviceTokenManager.saveDeviceToken(deviceToken = deviceToken) | |
| } catch (e: Exception) { | |
| // Log error | |
| } finally { | |
| pendingResult.finish() | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/dev/kotlin/team/aliens/dms/android/app/service/DmsMessagingService.kt
around lines 25-27, replace the unmanaged CoroutineScope(Dispatchers.IO).launch
call with a pattern that uses goAsync() to extend the service lifetime: call
goAsync() to obtain a PendingResult, start a coroutine on Dispatchers.IO to call
deviceTokenManager.saveDeviceToken(deviceToken), and ensure you always call
pendingResult.finish() in a finally block (and catch/log exceptions) so the work
completes even if the service is destroyed.
| override fun onMessageReceived(message: RemoteMessage) { | ||
| super.onMessageReceived(message) | ||
| message.notification?.run { | ||
| notificationManager.setNotificationContent( | ||
| title = title, | ||
| body = body, | ||
| ) | ||
| } | ||
| notificationManager.sendNotification() | ||
| } |
There was a problem hiding this comment.
Fix notification logic to prevent sending with stale content.
The code calls sendNotification() unconditionally on line 38, even when message.notification is null. This means a notification could be sent with stale or uninitialized content.
Apply this diff to only send notifications when content is present:
override fun onMessageReceived(message: RemoteMessage) {
super.onMessageReceived(message)
message.notification?.run {
notificationManager.setNotificationContent(
title = title,
body = body,
)
+ notificationManager.sendNotification()
}
- notificationManager.sendNotification()
}Alternatively, if you need to handle data messages differently, add explicit logic:
override fun onMessageReceived(message: RemoteMessage) {
super.onMessageReceived(message)
- message.notification?.run {
+ val notification = message.notification
+ if (notification != null) {
- notificationManager.setNotificationContent(
- title = title,
- body = body,
- )
+ notificationManager.setNotificationContent(
+ title = notification.title,
+ body = notification.body,
+ )
+ notificationManager.sendNotification()
+ } else {
+ // Handle data-only messages if needed
}
- notificationManager.sendNotification()
}📝 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.
| override fun onMessageReceived(message: RemoteMessage) { | |
| super.onMessageReceived(message) | |
| message.notification?.run { | |
| notificationManager.setNotificationContent( | |
| title = title, | |
| body = body, | |
| ) | |
| } | |
| notificationManager.sendNotification() | |
| } | |
| override fun onMessageReceived(message: RemoteMessage) { | |
| super.onMessageReceived(message) | |
| message.notification?.run { | |
| notificationManager.setNotificationContent( | |
| title = title, | |
| body = body, | |
| ) | |
| notificationManager.sendNotification() | |
| } | |
| } |
| override fun onMessageReceived(message: RemoteMessage) { | |
| super.onMessageReceived(message) | |
| message.notification?.run { | |
| notificationManager.setNotificationContent( | |
| title = title, | |
| body = body, | |
| ) | |
| } | |
| notificationManager.sendNotification() | |
| } | |
| override fun onMessageReceived(message: RemoteMessage) { | |
| super.onMessageReceived(message) | |
| val notification = message.notification | |
| if (notification != null) { | |
| notificationManager.setNotificationContent( | |
| title = notification.title, | |
| body = notification.body, | |
| ) | |
| notificationManager.sendNotification() | |
| } else { | |
| // Handle data-only messages if needed | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/dev/kotlin/team/aliens/dms/android/app/service/DmsMessagingService.kt
around lines 30 to 39, the code always calls
notificationManager.sendNotification() even when message.notification is null
causing possible stale/uninitialized notification content; fix by only calling
sendNotification() when you have non-null notification content (move the
sendNotification() call inside the message.notification?.run { ... } block) or,
if data messages must trigger notifications, add an explicit else branch to
build/clear notification content (or return) before calling sendNotification().
| @Provides | ||
| @Singleton | ||
| fun provideMoshi(): Moshi = Moshi.Builder().add(KotlinJsonAdapterFactory()).build() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine build configuration files
fd -E '.git' -t f \( -name "build.gradle" -o -name "build.gradle.kts" \) | head -20Repository: team-aliens/DMS-Android
Length of output: 297
🏁 Script executed:
# Check for Moshi-related dependencies
rg "moshi" --type gradle -iRepository: team-aliens/DMS-Android
Length of output: 215
🏁 Script executed:
# Check for ProGuard/R8 configuration files
fd -E '.git' -t f \( -name "proguard*.pro" -o -name "*.pro" -o -name "consumer-rules.pro" \)Repository: team-aliens/DMS-Android
Length of output: 297
🏁 Script executed:
# Examine the SerializationModule.kt file in detail
cat -n app/src/prod/kotlin/team/aliens/dms/android/app/di/serialization/SerializationModule.ktRepository: team-aliens/DMS-Android
Length of output: 692
🏁 Script executed:
# Find build.gradle files (simpler approach)
find . -type f -name "build.gradle*" | grep -v ".git" | head -20Repository: team-aliens/DMS-Android
Length of output: 656
🏁 Script executed:
# Search for moshi in build files
find . -type f \( -name "build.gradle" -o -name "build.gradle.kts" \) -exec grep -l -i moshi {} \;Repository: team-aliens/DMS-Android
Length of output: 150
🏁 Script executed:
# Find all ProGuard configuration files
find . -type f -name "*.pro" | grep -v ".git"Repository: team-aliens/DMS-Android
Length of output: 1339
🏁 Script executed:
# Search for moshi dependencies across gradle files
grep -r "moshi" . --include="build.gradle" --include="build.gradle.kts" 2>/dev/null | head -30Repository: team-aliens/DMS-Android
Length of output: 422
🏁 Script executed:
# Check app/build.gradle.kts for complete Moshi setup
cat app/build.gradle.kts | grep -A 5 -B 5 "moshi"Repository: team-aliens/DMS-Android
Length of output: 345
🏁 Script executed:
# Check ProGuard rules in app/proguard-rules.pro for Moshi
cat app/proguard-rules.pro | grep -i "moshi"Repository: team-aliens/DMS-Android
Length of output: 49
🏁 Script executed:
# Search for all Moshi builder usages in the codebase
grep -r "Moshi\.Builder" . --include="*.kt" 2>/dev/nullRepository: team-aliens/DMS-Android
Length of output: 421
🏁 Script executed:
# Check if codegen is actually being used with generated adapters
grep -r "JsonAdapter" . --include="*.kt" 2>/dev/null | grep -i "generated" | head -10Repository: team-aliens/DMS-Android
Length of output: 49
Replace reflection-based KotlinJsonAdapterFactory with Moshi code generation.
The project has moshi-kotlin-codegen configured with KSP as a dependency, but the SerializationModule uses the reflection-based KotlinJsonAdapterFactory instead. This creates a configuration mismatch and misses the benefits of compile-time code generation: better runtime performance, smaller APK size, and better R8 compatibility.
Additionally, no ProGuard/R8 rules are configured for Moshi, which is required for the reflection-based approach to work correctly after minification.
Switch to code-generated adapters: generate adapters with @JsonClass(generateAdapter = true) on data classes and use them directly in the Moshi builder, or remove the unused moshi-kotlin-codegen dependency if reflection is intentional.
🤖 Prompt for AI Agents
In
app/src/prod/kotlin/team/aliens/dms/android/app/di/serialization/SerializationModule.kt
around lines 15-17, the module currently registers the reflection-based
KotlinJsonAdapterFactory while the project is configured to use
moshi-kotlin-codegen; remove the KotlinJsonAdapterFactory from the Moshi builder
and switch to code-generated adapters by annotating all serializable data
classes with @JsonClass(generateAdapter = true) (or if you intentionally want
reflection, remove the moshi-kotlin-codegen dependency and add the required
ProGuard/R8 keep rules for Moshi); ensure DI provides a Moshi instance that does
not rely on KotlinJsonAdapterFactory when codegen is used so generated adapters
are used at runtime.
There was a problem hiding this comment.
Pull request overview
This PR implements build flavors (dev and prod) to separate the existing production code from new development work. It splits the app module into two distinct flavors, each with its own source sets, dependencies, and configurations, allowing parallel development without code deletion.
Key Changes:
- Configured Gradle build flavors with separate
devandprodproduct flavors - Split dependencies to use flavor-specific implementations (e.g.,
prodImplementationfor Compose Destinations,devImplementationfor experimental navigation3) - Separated all app-level files into flavor-specific source directories
Reviewed changes
Copilot reviewed 14 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
app/build.gradle.kts |
Configures build flavors with dimension, source sets, and flavor-specific dependencies |
app/src/prod/AndroidManifest.xml |
Production flavor manifest specifying DmsApplication |
app/src/prod/kotlin/team/aliens/dms/android/app/* |
Production app implementation with full navigation and feature integration |
app/src/dev/AndroidManifest.xml |
Development flavor manifest with DevApplication and MainActivity configuration |
app/src/dev/kotlin/team/aliens/dms/android/app/* |
Development app implementation using experimental navigation3 library with simplified setup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
app/src/dev/kotlin/team/aliens/dms/android/app/EdgeToEdgeConfig.kt
Outdated
Show resolved
Hide resolved
| } | ||
| notificationManager.sendNotification() | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Missing trailing newline at the end of the file. The dev flavor's DmsMessagingService is missing the trailing newline that exists in the prod version (line 41).
| windowSizeClass: WindowSizeClass, | ||
| // displayFeatures: List<DisplayFeature>, | ||
| // isJwtAvailable: StateFlow<Boolean>, | ||
| // appState: DmsAppState = rememberDmsAppState( | ||
| // isJwtAvailable = isJwtAvailable, | ||
| // ), |
There was a problem hiding this comment.
The commented-out parameters and functionality suggest incomplete implementation. These commented lines (displayFeatures, isJwtAvailable, appState) should either be removed if not needed, or uncommented and properly implemented if they are intended for future use. Consider adding a TODO comment explaining why this is a simplified version compared to the prod flavor.
|
|
||
| DmsTheme { | ||
| DmsApp( | ||
| windowSizeClass = windowSizeClass, |
There was a problem hiding this comment.
The DmsApp composable is called without the required isJwtAvailable parameter that is injected in the MainActivity. In the prod version, this parameter is passed to DmsApp. Either the parameter should be passed here, or the dev DmsApp signature should be updated to not expect it (which appears to be the case based on the commented-out parameters).
| windowSizeClass = windowSizeClass, | |
| windowSizeClass = windowSizeClass, | |
| isJwtAvailable = isJwtAvailable, |
app/src/dev/AndroidManifest.xml
Outdated
| tools:replace="android:name"> | ||
|
|
||
| <activity | ||
| android:name=".android.MainActivity" |
There was a problem hiding this comment.
The activity name path is incorrect. It should be team.aliens.dms.android.app.MainActivity instead of .android.MainActivity. The current path would resolve to team.aliens.dms.android.MainActivity which doesn't match the actual package location of the MainActivity class (team.aliens.dms.android.app).
| android:name=".android.MainActivity" | |
| android:name="team.aliens.dms.android.app.MainActivity" |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt (1)
68-70: (Previously noted) Add error handling aroundfetchDeviceToken().This coroutine still calls
deviceTokenManager.fetchDeviceToken()without handling exceptions. If that call can fail, consider wrapping it and logging/reporting the error so a transient issue doesn’t crash the dev build.lifecycleScope.launch { - deviceTokenManager.fetchDeviceToken() + runCatching { + deviceTokenManager.fetchDeviceToken() + }.onFailure { throwable -> + // TODO: log or report token fetch failure + } }
🧹 Nitpick comments (3)
app/src/dev/kotlin/team/aliens/dms/android/app/EdgeToEdgeConfig.kt (1)
7-13: Edge-to-edge helper looks correct; consider reusing across variants.The version-gated
isNavigationBarContrastEnforcedchange is appropriate for Q+ and the extension cleanly encapsulates the window setup. If you need the same behavior in prod activities, consider moving this helper to a common source set (or invoking it fromMainActivity) so the edge-to-edge configuration isn’t duplicated per flavor.app/src/prod/kotlin/team/aliens/dms/android/app/MainActivity.kt (2)
68-70: Handle failures when fetching the device token.
fetchDeviceToken()is invoked inlifecycleScope.launchwithout any error handling. If this call can throw (network/Play Services issues, etc.), it may crash the activity. Wrapping it defensively and logging/reporting failures would make this path more robust.- lifecycleScope.launch { - deviceTokenManager.fetchDeviceToken() - } + lifecycleScope.launch { + runCatching { + deviceTokenManager.fetchDeviceToken() + }.onFailure { throwable -> + // TODO: log or report token fetch failure + } + }
73-95: Consider also checking whether IMMEDIATE updates are allowed before starting the flow.You already guard on update availability and route failures to
mainViewModel.onUpdateFailed(). For in-app updates it’s also common to check that the requested update type is allowed before starting the flow, to avoid immediately hitting the failure callback.For example:
- appUpdateInfoTask.addOnSuccessListener { appUpdateInfo -> - if (appUpdateInfo.updateAvailability() != UpdateAvailability.UPDATE_AVAILABLE) { - return@addOnSuccessListener - } + appUpdateInfoTask.addOnSuccessListener { appUpdateInfo -> + val isUpdateAvailable = + appUpdateInfo.updateAvailability() == UpdateAvailability.UPDATE_AVAILABLE + val isImmediateAllowed = + appUpdateInfo.isUpdateTypeAllowed(AppUpdateType.IMMEDIATE) + if (!isUpdateAvailable || !isImmediateAllowed) { + return@addOnSuccessListener + }Not required for correctness (you already handle failures), but it can reduce unnecessary attempts.
Please double-check the latest Play Core in-app update guidelines to confirm this pattern matches current recommendations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/src/dev/AndroidManifest.xml(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/EdgeToEdgeConfig.kt(1 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt(6 hunks)app/src/dev/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/MainActivity.kt(1 hunks)app/src/prod/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/dev/AndroidManifest.xml
🧰 Additional context used
🧬 Code graph analysis (2)
app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Buttons.kt (1)
Button(42-92)
app/src/prod/kotlin/team/aliens/dms/android/app/MainActivity.kt (3)
app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt (1)
checkAppUpdate(73-95)core/design-system/src/main/java/team/aliens/dms/android/core/designsystem/Theme.kt (1)
DmsTheme(11-35)app/src/prod/kotlin/team/aliens/dms/android/app/DmsApp.kt (1)
DmsApp(34-95)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (5)
app/src/prod/kotlin/team/aliens/dms/android/app/MainActivity.kt (1)
28-66: MainActivity wiring and DI setup look solid.The ViewModel injection, JWT availability flow, update launcher, and composition of
DmsAppare all consistent and keep the prod flavor aligned with the dev setup. No functional issues spotted in this section.app/src/dev/kotlin/team/aliens/dms/android/app/MainActivity.kt (2)
59-64: DmsApp call in dev flavor is correctly wired.Passing both
isJwtAvailableandmainViewModelkeeps the dev MainActivity aligned with the prod flavor and ensures the devDmsApphas everything it needs for navigation/auth and update-failure UI.
77-94: Update availability guard and failure routing look good.The added early return when no update is available plus the failure listener that delegates to
mainViewModel.onUpdateFailed()provide a clean, observable contract for the UI without unnecessary update attempts.app/src/dev/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt (1)
11-26: Update-failure ViewModel state is cleanly modeled.Using a private backing flag with
onUpdateFailed()andconsumeUpdateFailed()provides a straightforward way for the UI to react once to an update failure and then reset the state. This keeps the dev ViewModel aligned with the prod counterpart.app/src/prod/kotlin/team/aliens/dms/android/app/MainActivityViewModel.kt (1)
15-24: Prod ViewModel’s update-failure API matches dev and looks good.The added
_isUpdateFailedflag, publicisUpdateFailed, and theonUpdateFailed()/consumeUpdateFailed()pair provide a clear contract for the UI to observe and then clear update-failure events, keeping parity with the dev flavor.
| val isUpdateFailed by mainViewModel.isUpdateFailed.collectAsState() | ||
| val toast = LocalToast.current | ||
|
|
||
| if (isUpdateFailed) { | ||
| LaunchedEffect(Unit) { | ||
| toast.showErrorToast( | ||
| message = "업데이트 정보를 불러올 수 없습니다", | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the DmsApp.kt file and read the full context
fd -t f "DmsApp.kt" | head -5Repository: team-aliens/DMS-Android
Length of output: 181
🏁 Script executed:
# Read the file to understand the full context
head -80 "app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt"Repository: team-aliens/DMS-Android
Length of output: 2497
🏁 Script executed:
# Search for mainViewModel type and consumeUpdateFailed method
rg "consumeUpdateFailed" -A 3 -B 3Repository: team-aliens/DMS-Android
Length of output: 1328
🏁 Script executed:
# Find the mainViewModel definition and its type
rg "mainViewModel.*=" "app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt" -A 2Repository: team-aliens/DMS-Android
Length of output: 49
🏁 Script executed:
# Search for isUpdateFailed definition in the codebase
rg "isUpdateFailed" -B 2 -A 2 | head -100Repository: team-aliens/DMS-Android
Length of output: 3368
🏁 Script executed:
# Search for similar LaunchedEffect patterns with conditionals in Compose code
rg "LaunchedEffect\(Unit\)" -B 3 -A 3 | head -60Repository: team-aliens/DMS-Android
Length of output: 6548
🏁 Script executed:
# Search for other event consumption patterns to see if this pattern is used elsewhere
rg "consume" -t kt --max-count 20Repository: team-aliens/DMS-Android
Length of output: 93
isUpdateFailed flag is never consumed, preventing subsequent failures from being handled.
The current code collects isUpdateFailed and shows a toast inside LaunchedEffect(Unit) within an if block. Because LaunchedEffect(Unit) uses a constant key, it runs only once when the condition first becomes true. Since consumeUpdateFailed() is never called, the flag remains true indefinitely, and subsequent failures cannot trigger new toasts.
To handle each failure as a distinct event, react to flag changes and consume immediately:
- val isUpdateFailed by mainViewModel.isUpdateFailed.collectAsState()
- val toast = LocalToast.current
-
- if (isUpdateFailed) {
- LaunchedEffect(Unit) {
- toast.showErrorToast(
- message = "업데이트 정보를 불러올 수 없습니다",
- )
- }
- }
+ val isUpdateFailed by mainViewModel.isUpdateFailed.collectAsState()
+ val toast = LocalToast.current
+
+ LaunchedEffect(isUpdateFailed) {
+ if (isUpdateFailed) {
+ toast.showErrorToast(
+ message = "업데이트 정보를 불러올 수 없습니다",
+ )
+ mainViewModel.consumeUpdateFailed()
+ }
+ }This way each failure triggers exactly one toast and then resets the flag so later failures can be detected and handled again.
📝 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.
| val isUpdateFailed by mainViewModel.isUpdateFailed.collectAsState() | |
| val toast = LocalToast.current | |
| if (isUpdateFailed) { | |
| LaunchedEffect(Unit) { | |
| toast.showErrorToast( | |
| message = "업데이트 정보를 불러올 수 없습니다", | |
| ) | |
| } | |
| } | |
| val isUpdateFailed by mainViewModel.isUpdateFailed.collectAsState() | |
| val toast = LocalToast.current | |
| LaunchedEffect(isUpdateFailed) { | |
| if (isUpdateFailed) { | |
| toast.showErrorToast( | |
| message = "업데이트 정보를 불러올 수 없습니다", | |
| ) | |
| mainViewModel.consumeUpdateFailed() | |
| } | |
| } |
🤖 Prompt for AI Agents
In app/src/dev/kotlin/team/aliens/dms/android/app/DmsApp.kt around lines 40 to
49, the isUpdateFailed flag is collected but never consumed and the
LaunchedEffect uses a constant key so the toast only triggers once; change the
effect to react to the flag value (e.g., LaunchedEffect(isUpdateFailed) or
collectAsState changes) and immediately call the ViewModel method to
consume/reset the flag (e.g., mainViewModel.consumeUpdateFailed()) after showing
the toast so each subsequent failure becomes a new event and will display its
own toast.
개요
작업사항
추가 로 할 말
Summary by CodeRabbit
New Features
Bug Fixes / Stability
✏️ Tip: You can customize this high-level summary in your review settings.