Adding compose code for FCM by lehcar09 · Pull Request #1632 · firebase/quickstart-android

lehcar09 · 2024-06-10T15:09:29Z

Changes made:

Add Jetpack Compose option in EntryChoiceActivity
Removed business logic from MainActivity
Added FirebaseMessagingViewModel for business logic
Added ComposeMainActivity for the Jetpack Compose UI

thatfiredev

Thanks for working on this @lehcar09 ! I have added some comments/suggestions below

...ng/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/MyFirebaseMessagingService.kt

messaging/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/ComposeMainActivity.kt

...ng/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/FirebaseMessagingViewModel.kt

thatfiredev · 2026-03-06T23:30:41Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a Jetpack Compose version of the FCM quickstart UI, refactoring the MainActivity to use a new FirebaseMessagingViewModel for business logic, which improves separation of concerns and modernizes the UI. A critical security concern is the logging of sensitive information, such as the FCM registration token and intent extras, to Logcat by FirebaseMessagingViewModel, which could lead to data leakage. These log statements should be removed or sanitized. Furthermore, several issues were identified, including a critical XML syntax error in strings.xml, incomplete Material Theming adoption in Compose, a ViewModel exposing mutable state flow, a likely Compose preview crash, and opportunities for LaunchedEffect and minor code style improvements.

gemini-code-assist · 2026-03-06T23:36:42Z

messaging/app/src/main/res/values/strings.xml

+    <string name="msg_permission_granted">Notifications permission granted</string>
+    <string name="msg_permission_failed">FCM can\'t post notifications without POST_NOTIFICATIONS permission</string>
+
+    <string name="msg_setup_readme_instructions">See README for setup instructions</string>"


There's a stray double-quote character at the end of this line, which will cause an XML parsing error and fail the build.

Suggested change

<string name="msg_setup_readme_instructions">See README for setup instructions</string>"

<string name="msg_setup_readme_instructions">See README for setup instructions</string>

gemini-code-assist · 2026-03-06T23:36:42Z

...ng/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/FirebaseMessagingViewModel.kt

+        intent.extras?.let {
+            it.keySet().forEach { key ->
+                val value = intent.extras?.getString(key)
+                Log.d(TAG, "Key: $key Value: $value")


The logNotificationData function logs all extras from the incoming intent. Intent extras can contain sensitive information from FCM data payloads. Logging this data to Logcat can lead to information leakage, as Logcat is accessible to other applications and via ADB. It is recommended to only log specific, non-sensitive fields or to remove this logging in production builds.

gemini-code-assist · 2026-03-06T23:36:42Z

...ng/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/FirebaseMessagingViewModel.kt

+        viewModelScope.launch {
+            try {
+                _token.value = messaging.getToken().await()
+                Log.d(TAG, "FCM registration Token: ${_token.value}")


The getToken function logs the FCM registration token. FCM tokens are sensitive credentials that identify a specific app instance and can be used to send targeted notifications. Logging them to Logcat is a security risk as it exposes these credentials to anyone with access to the device logs. This logging should be removed or restricted to debug builds only.

gemini-code-assist · 2026-03-06T23:36:42Z

messaging/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/ComposeMainActivity.kt

+        var message = context.getString(R.string.msg_permission_granted)
+        if (!isGranted) {
+            message = context.getString(R.string.msg_permission_failed)
+        }


The message variable is assigned once and never changed within its scope. It can be declared as a val using an if-else expression, which is better for readability and immutability.

Suggested change

var message = context.getString(R.string.msg_permission_granted)

if (!isGranted) {

message = context.getString(R.string.msg_permission_failed)

}

val message = if (isGranted) {

context.getString(R.string.msg_permission_granted)

} else {

context.getString(R.string.msg_permission_failed)

}

gemini-code-assist · 2026-03-06T23:36:42Z

messaging/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/ComposeMainActivity.kt

+    DisposableEffect(lifecycleOwner) {
+        val observer = LifecycleEventObserver { _, event ->
+            if (event == Lifecycle.Event.ON_CREATE) {
+                //Create Notification Channel
+                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+                    fcmViewModel.setNotificationChannel(
+                        context,
+                        context.getString(R.string.default_notification_channel_id),
+                        context.getString(R.string.default_notification_channel_name)
+                    )
+                }
+
+                intent?.let { intentData ->
+                    fcmViewModel.logNotificationData(intentData)
+                }
+
+                scope.launch {
+                    snackbarHostState.showSnackbar(context.getString(R.string.msg_setup_readme_instructions))
+                }
+
+                fcmViewModel.askNotificationPermission(context, requestPermissionLauncher)
+            }
+        }
+
+        lifecycleOwner.lifecycle.addObserver(observer)
+
+        onDispose {
+            lifecycleOwner.lifecycle.removeObserver(observer)
+        }
+    }


Using DisposableEffect with Lifecycle.Event.ON_CREATE for one-time setup can lead to the code running multiple times, for example on configuration changes. A more idiomatic and safer way to perform one-time setup in Compose is to use LaunchedEffect(Unit). This ensures the code runs only once when the composable enters the composition.

LaunchedEffect(Unit) { //Create Notification Channel if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { fcmViewModel.setNotificationChannel( context, context.getString(R.string.default_notification_channel_id), context.getString(R.string.default_notification_channel_name) ) } intent?.let { intentData -> fcmViewModel.logNotificationData(intentData) } snackbarHostState.showSnackbar(context.getString(R.string.msg_setup_readme_instructions)) fcmViewModel.askNotificationPermission(context, requestPermissionLauncher) }

gemini-code-assist · 2026-03-06T23:36:42Z

messaging/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/ComposeMainActivity.kt

+            .fillMaxSize(),
+        topBar = {
+            TopAppBar(
+                backgroundColor = colorResource(R.color.colorPrimary)


The TopAppBar's background color is hardcoded using colorResource. To better support theming (including dark theme), you should use colors from MaterialTheme.colors. The Theme.kt file already defines FirebaseBlue as the primary color for the light theme.

backgroundColor = MaterialTheme.colors.primary

gemini-code-assist · 2026-03-06T23:36:42Z

messaging/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/ComposeMainActivity.kt

+                .width(200.dp)
+                .wrapContentHeight()
+                .padding(0.dp, 20.dp, 0.dp, 0.dp),
+            colors = ButtonDefaults.buttonColors(backgroundColor = colorResource(R.color.colorPrimary)),


The Button's background color is hardcoded using colorResource. To better support theming (including dark theme), you should use colors from MaterialTheme.colors. The Theme.kt file already defines FirebaseBlue as the primary color for the light theme. This also applies to the other button.

colors = ButtonDefaults.buttonColors(backgroundColor = MaterialTheme.colors.primary),

gemini-code-assist · 2026-03-06T23:36:43Z

messaging/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/ComposeMainActivity.kt

+fun MainAppViewPreview() {
+    FirebaseMessagingTheme {
+        MainScreen()
+    }


This preview will likely crash because MainScreen's default viewModel() call will try to instantiate FirebaseMessagingViewModel. The factory for this ViewModel calls FirebaseMessaging.getInstance(), which is not available in a preview context. To make previews work, you should provide a mock or fake implementation of the ViewModel.

gemini-code-assist · 2026-03-06T23:36:43Z

...ng/app/src/main/java/com/google/firebase/quickstart/fcm/kotlin/FirebaseMessagingViewModel.kt

+    private var _token: MutableStateFlow<String> = MutableStateFlow("")
+    val token: MutableStateFlow<String> = _token


The _token MutableStateFlow is exposed publicly as mutable. To follow best practices for encapsulation and prevent unintended state modifications from outside the ViewModel, you should expose it as an immutable StateFlow. Also, _token can be a val since the MutableStateFlow instance itself is not reassigned.

Suggested change

private var _token: MutableStateFlow<String> = MutableStateFlow("")

val token: MutableStateFlow<String> = _token

private val _token = MutableStateFlow("")

val token: StateFlow<String> = _token

lehcar09 added 2 commits June 10, 2024 23:08

- Initial Commit

c3cde89

- Adding compose for FCM

ba6914a

lehcar09 changed the title ~~- Initial Commit~~ Adding compose code for FCM Jun 10, 2024

thatfiredev mentioned this pull request Jun 14, 2024

Create Jetpack Compose versions of the quickstarts #1457

Open

- Update compose preview

a89205d

lehcar09 requested a review from thatfiredev July 19, 2024 12:27

thatfiredev reviewed Aug 19, 2024

View reviewed changes

lehcar09 added 2 commits August 21, 2024 17:50

Resolve review comments/ suggestions

9d33039

Remove accompanist dependency

4eba4a9

gemini-code-assist bot reviewed Mar 6, 2026

View reviewed changes

	<string name="msg_setup_readme_instructions">See README for setup instructions</string>"
	<string name="msg_setup_readme_instructions">See README for setup instructions</string>

		private var _token: MutableStateFlow<String> = MutableStateFlow("")
		val token: MutableStateFlow<String> = _token

Conversation

lehcar09 commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatfiredev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thatfiredev commented Mar 6, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

lehcar09 commented Jun 10, 2024 •

edited

Loading