fix(car): address code review findings — lifecycle, security, thread safety

Critical fixes:
- EmergencyHandler: recreate scope on startCollecting() (singleton was permanently dead after first session)
- MeshtasticCarAppService: gate ALLOW_ALL_HOSTS_VALIDATOR behind BuildConfig.DEBUG, add hosts_allowlist.xml for production

High fixes:
- MeshtasticCarSession: wire destroy() via DefaultLifecycleObserver on ON_DESTROY
- MeshStatusPanel: remove unused CoroutineScope (no coroutines launched)
- CarStateCoordinator: use MutableStateFlow for channel index, ConcurrentHashMap for messagesCache
- CarTtsEngine: wire shutdown() call into coordinator destroy()

Medium fixes:
- NodeDetailScreen: replace all hardcoded strings with R.string resources
- DisconnectedScreen: replace hardcoded strings with resources
- Add comprehensive string resources for time formatting and status labels

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
James Rich
2026-05-21 19:58:06 -05:00
parent 1f85789320
commit af55e642cd
11 changed files with 136 additions and 57 deletions

View File

@@ -24,6 +24,8 @@ plugins {
android {
namespace = "org.meshtastic.feature.car"
buildFeatures { buildConfig = true }
defaultConfig {
minSdk = 23
consumerProguardFiles("proguard-rules.pro")

View File

@@ -37,23 +37,28 @@ import org.meshtastic.feature.car.model.EmergencyAlert
@Single
class EmergencyHandler {
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)
private var scope: CoroutineScope? = null
private val _activeAlerts = MutableStateFlow<List<EmergencyAlert>>(emptyList())
val activeAlerts: StateFlow<List<EmergencyAlert>> = _activeAlerts.asStateFlow()
private var toneGenerator: ToneGenerator? = null
fun startCollecting(emergencyFlow: Flow<EmergencyAlert>) {
scope.launch {
emergencyFlow.collect { alert ->
addAlert(alert)
playEmergencyTone()
scope?.cancel()
scope =
CoroutineScope(SupervisorJob() + Dispatchers.Main).also { newScope ->
newScope.launch {
emergencyFlow.collect { alert ->
addAlert(alert)
playEmergencyTone()
}
}
}
}
}
fun stopCollecting() {
scope.cancel()
scope?.cancel()
scope = null
toneGenerator?.release()
toneGenerator = null
}

View File

@@ -16,10 +16,6 @@
*/
package org.meshtastic.feature.car.panels
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.cancel
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
@@ -34,7 +30,6 @@ import org.meshtastic.feature.car.model.ConnectionStatus
@Single
class MeshStatusPanel {
private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Main)
private val _state =
MutableStateFlow(
CarSessionState(
@@ -86,10 +81,6 @@ class MeshStatusPanel {
return "Last msg: $timeAgo"
}
fun destroy() {
scope.cancel()
}
companion object {
private const val MILLIS_PER_MINUTE = 60_000L
private const val MILLIS_PER_HOUR = 3_600_000L

View File

@@ -45,6 +45,5 @@ class MeshStatusSessionWiring(private val panel: MeshStatusPanel) {
connectionJob?.cancel()
nodeCountJob?.cancel()
messageTimeJob?.cancel()
panel.destroy()
}
}

View File

@@ -37,13 +37,13 @@ class DisconnectedScreen(carContext: CarContext) : Screen(carContext) {
.addRow(
Row.Builder()
.setTitle(carContext.getString(R.string.car_disconnected))
.addText("Radio connection lost. Showing cached data.")
.addText(carContext.getString(R.string.car_disconnected_body))
.build(),
)
.addRow(
Row.Builder()
.setTitle("Reconnecting...")
.addText("The app will automatically reconnect when the radio is available.")
.setTitle(carContext.getString(R.string.car_reconnecting_title))
.addText(carContext.getString(R.string.car_reconnecting_body))
.build(),
)
.build(),

View File

@@ -39,15 +39,38 @@ class NodeDetailScreen(
val paneBuilder = Pane.Builder()
paneBuilder.addRow(Row.Builder().setTitle("Signal").addText(formatSignal(node.signalQuality)).build())
paneBuilder.addRow(
Row.Builder()
.setTitle(carContext.getString(R.string.car_status_signal))
.addText(formatSignal(node.signalQuality))
.build(),
)
node.batteryPercent?.let { battery ->
paneBuilder.addRow(Row.Builder().setTitle("Battery").addText("$battery%").build())
paneBuilder.addRow(
Row.Builder().setTitle(carContext.getString(R.string.car_status_battery)).addText("$battery%").build(),
)
}
paneBuilder.addRow(Row.Builder().setTitle("Last Heard").addText(formatLastHeard(node.lastHeard)).build())
paneBuilder.addRow(
Row.Builder()
.setTitle(carContext.getString(R.string.car_status_last_heard))
.addText(formatLastHeard(node.lastHeard))
.build(),
)
paneBuilder.addRow(Row.Builder().setTitle("Status").addText(if (node.isOnline) "Online" else "Offline").build())
paneBuilder.addRow(
Row.Builder()
.setTitle(carContext.getString(R.string.car_status_status))
.addText(
if (node.isOnline) {
carContext.getString(R.string.car_status_online)
} else {
carContext.getString(R.string.car_status_offline)
},
)
.build(),
)
paneBuilder.addAction(
Action.Builder()
@@ -61,10 +84,18 @@ class NodeDetailScreen(
.build()
}
private fun buildErrorTemplate(): Template =
PaneTemplate.Builder(Pane.Builder().addRow(Row.Builder().setTitle("Node not found").build()).build())
.setHeader(Header.Builder().setTitle("Error").setStartHeaderAction(Action.BACK).build())
.build()
private fun buildErrorTemplate(): Template = PaneTemplate.Builder(
Pane.Builder()
.addRow(Row.Builder().setTitle(carContext.getString(R.string.car_node_not_found)).build())
.build(),
)
.setHeader(
Header.Builder()
.setTitle(carContext.getString(R.string.car_error))
.setStartHeaderAction(Action.BACK)
.build(),
)
.build()
private fun formatSignal(quality: SignalQuality): String = when (quality) {
SignalQuality.EXCELLENT -> carContext.getString(R.string.car_signal_excellent)
@@ -75,13 +106,18 @@ class NodeDetailScreen(
}
private fun formatLastHeard(epochMillis: Long): String {
if (epochMillis == 0L) return "Never"
if (epochMillis == 0L) return carContext.getString(R.string.car_time_never)
val elapsed = System.currentTimeMillis() - epochMillis
return when {
elapsed < MILLIS_PER_MINUTE -> "Just now"
elapsed < MILLIS_PER_HOUR -> "${elapsed / MILLIS_PER_MINUTE}m ago"
elapsed < MILLIS_PER_DAY -> "${elapsed / MILLIS_PER_HOUR}h ago"
else -> "${elapsed / MILLIS_PER_DAY}d ago"
elapsed < MILLIS_PER_MINUTE -> carContext.getString(R.string.car_time_just_now)
elapsed < MILLIS_PER_HOUR ->
carContext.getString(R.string.car_time_minutes_ago, (elapsed / MILLIS_PER_MINUTE).toInt())
elapsed < MILLIS_PER_DAY ->
carContext.getString(R.string.car_time_hours_ago, (elapsed / MILLIS_PER_HOUR).toInt())
else -> carContext.getString(R.string.car_time_days_ago, (elapsed / MILLIS_PER_DAY).toInt())
}
}

View File

@@ -49,6 +49,7 @@ import org.meshtastic.feature.car.model.NodeUi
import org.meshtastic.feature.car.model.SignalQuality
import org.meshtastic.feature.car.model.TopologyHeader
import org.meshtastic.feature.car.util.CarTtsEngine
import java.util.concurrent.ConcurrentHashMap
/** Snapshot of a message for car display (avoids leaking domain models to UI). */
data class MessageSnapshot(
@@ -106,7 +107,7 @@ class CarStateCoordinator(
private val _quickChatActions = MutableStateFlow<List<String>>(emptyList())
val quickChatActions: StateFlow<List<String>> = _quickChatActions.asStateFlow()
private var selectedChannelIndex = 0
private val selectedChannel = MutableStateFlow(0)
fun start() {
collectConnectionState()
@@ -116,7 +117,7 @@ class CarStateCoordinator(
}
fun selectChannel(index: Int) {
selectedChannelIndex = index
selectedChannel.value = index
_messagingState.value = _messagingState.value.copy(selectedChannelIndex = index)
}
@@ -145,7 +146,7 @@ class CarStateCoordinator(
to = contactKey,
bytes = text.encodeToByteArray().toByteString(),
dataType = DATA_TYPE_TEXT,
channel = selectedChannelIndex,
channel = selectedChannel.value,
)
commandSender.sendData(packet)
}
@@ -159,7 +160,7 @@ class CarStateCoordinator(
}
}
private val messagesCache = mutableMapOf<String, List<MessageSnapshot>>()
private val messagesCache = ConcurrentHashMap<String, List<MessageSnapshot>>()
fun cacheMessages(contactKey: String, messages: List<MessageSnapshot>) {
messagesCache[contactKey] = messages
@@ -172,6 +173,7 @@ class CarStateCoordinator(
fun destroy() {
scope.cancel()
ttsEngine.shutdown()
}
private fun collectConnectionState() {
@@ -243,7 +245,7 @@ class CarStateCoordinator(
_messagingState.value =
MessagingUiState(
channels = channels,
selectedChannelIndex = selectedChannelIndex,
selectedChannelIndex = selectedChannel.value,
conversations = conversations,
emergencySpotlight = null,
)

View File

@@ -20,10 +20,16 @@ import androidx.car.app.CarAppService
import androidx.car.app.Session
import androidx.car.app.SessionInfo
import androidx.car.app.validation.HostValidator
import org.meshtastic.feature.car.BuildConfig
import org.meshtastic.feature.car.R
class MeshtasticCarAppService : CarAppService() {
override fun createHostValidator(): HostValidator = HostValidator.ALLOW_ALL_HOSTS_VALIDATOR
override fun createHostValidator(): HostValidator = if (BuildConfig.DEBUG) {
HostValidator.ALLOW_ALL_HOSTS_VALIDATOR
} else {
HostValidator.Builder(applicationContext).addAllowedHosts(R.array.car_hosts_allowlist).build()
}
override fun onCreateSession(sessionInfo: SessionInfo): Session = MeshtasticCarSession()
}

View File

@@ -20,6 +20,8 @@ import android.content.Intent
import android.content.res.Configuration
import androidx.car.app.Screen
import androidx.car.app.Session
import androidx.lifecycle.DefaultLifecycleObserver
import androidx.lifecycle.LifecycleOwner
import kotlinx.coroutines.flow.emptyFlow
import org.koin.core.component.KoinComponent
import org.koin.core.component.inject
@@ -42,6 +44,15 @@ class MeshtasticCarSession :
stateCoordinator.start()
// Emergency flow wired to emptyFlow() until emergency packet detection is implemented
emergencyWiring.attach(emptyFlow())
lifecycle.addObserver(
object : DefaultLifecycleObserver {
override fun onDestroy(owner: LifecycleOwner) {
destroy()
}
},
)
return HomeScreen(carContext, stateCoordinator)
}
@@ -53,7 +64,7 @@ class MeshtasticCarSession :
// Handle theme/density changes — templates auto-update
}
fun destroy() {
private fun destroy() {
emergencyWiring.detach()
stateCoordinator.destroy()
crashlyticsCarTagger.setCarSession(false)

View File

@@ -0,0 +1,11 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<string-array name="car_hosts_allowlist">
<!-- Android Auto host (Google) -->
<item>com.google.android.projection.gearhead</item>
<!-- Android Automotive OS system host -->
<item>com.android.car.carlauncher</item>
<!-- Android Auto for Phone Screens (testing) -->
<item>com.google.android.apps.auto</item>
</string-array>
</resources>

View File

@@ -1,30 +1,46 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>
<string name="car_app_name">Meshtastic</string>
<string name="car_tab_messages">Messages</string>
<string name="car_tab_nodes">Nodes</string>
<string name="car_disconnected">Disconnected</string>
<string name="car_reconnecting">Radio connection lost. Will reconnect automatically.</string>
<string name="car_battery_level">Battery: %d%%</string>
<string name="car_connecting">Connecting…</string>
<string name="car_no_channels">No channels configured</string>
<string name="car_no_nodes">No nodes heard</string>
<string name="car_no_messages">No messages yet</string>
<string name="car_nodes_online">%d nodes online</string>
<string name="car_last_message">Last msg: %s</string>
<string name="car_disconnected">Disconnected</string>
<string name="car_disconnected_body">Radio connection lost. Showing cached data.</string>
<string name="car_emergency_alert">Emergency Alert</string>
<string name="car_voice_reply">Reply</string>
<string name="car_error">Error</string>
<string name="car_last_heard">Last heard: %s</string>
<string name="car_last_message">Last msg: %s</string>
<string name="car_message_node">Message</string>
<string name="car_message_too_long">Message exceeds 237 bytes</string>
<string name="car_no_channels">No channels configured</string>
<string name="car_no_messages">No messages yet</string>
<string name="car_no_nodes">No nodes heard</string>
<string name="car_node_not_found">Node not found</string>
<string name="car_nodes_online">%d nodes online</string>
<string name="car_onboarding_text">Open Meshtastic on your phone to configure channels and connect to a radio.</string>
<string name="car_onboarding_title">Setup Required</string>
<string name="car_quick_reply">Quick Reply</string>
<string name="car_read_aloud">Read Aloud</string>
<string name="car_message_node">Message</string>
<string name="car_reconnecting">Radio connection lost. Will reconnect automatically.</string>
<string name="car_reconnecting_title">Reconnecting…</string>
<string name="car_reconnecting_body">The app will automatically reconnect when the radio is available.</string>
<string name="car_signal_excellent">Excellent</string>
<string name="car_signal_good">Good</string>
<string name="car_signal_fair">Fair</string>
<string name="car_signal_good">Good</string>
<string name="car_signal_poor">Poor</string>
<string name="car_signal_unknown">Unknown</string>
<string name="car_battery_level">Battery: %d%%</string>
<string name="car_last_heard">Last heard: %s</string>
<string name="car_onboarding_title">Setup Required</string>
<string name="car_onboarding_text">Open Meshtastic on your phone to configure channels and connect to a radio.</string>
<string name="car_message_too_long">Message exceeds 237 bytes</string>
<string name="car_status_battery">Battery</string>
<string name="car_status_last_heard">Last Heard</string>
<string name="car_status_offline">Offline</string>
<string name="car_status_online">Online</string>
<string name="car_status_signal">Signal</string>
<string name="car_status_status">Status</string>
<string name="car_tab_messages">Messages</string>
<string name="car_tab_nodes">Nodes</string>
<string name="car_time_days_ago">%dd ago</string>
<string name="car_time_hours_ago">%dh ago</string>
<string name="car_time_just_now">Just now</string>
<string name="car_time_minutes_ago">%dm ago</string>
<string name="car_time_never">Never</string>
<string name="car_unread_badge">%d unread</string>
<string name="car_voice_reply">Reply</string>
</resources>