fix(charts): apply Vico 3.1.0 best-practice audit fixes (#5138)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
2026-04-20 22:23:37 +00:00 · 2026-04-14 21:20:33 -05:00 · 2026-04-14 21:20:33 -05:00 · a2763bdfeb
commit a2763bdfeb
parent 72b981f73b
6 changed files with 110 additions and 62 deletions
--- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/BaseMetricChart.kt
+++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/BaseMetricChart.kt
@ -35,6 +35,7 @@ import androidx.compose.material3.IconToggleButton
 import androidx.compose.material3.Scaffold
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.getValue
+import androidx.compose.runtime.key
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberCoroutineScope
@ -79,6 +80,9 @@ import org.meshtastic.core.ui.icon.MeshtasticIcons
 import org.meshtastic.core.ui.icon.Refresh
 import org.meshtastic.core.ui.icon.Save

+/** Minimum x-step (in seconds) to prevent the default GCD from producing a value of 1 with irregular timestamps. */
+private const val MIN_X_STEP_SECONDS = 60.0
+
 /**
 * A generic chart host for Meshtastic metric charts. Handles common boilerplate for markers, scrolling, and point
 * selection synchronization.
@ -100,43 +104,50 @@ fun GenericMetricChart(
    onPointSelected: ((Double) -> Unit)? = null,
    vicoScrollState: VicoScrollState = rememberVicoScrollState(),
 ) {
-    // Hoist zoom state above rememberCartesianChart so that the variable slot count
-    // from the vararg layers spread does not shift this remember call during recomposition
-    // (toggling legend chips changes the layer count, which corrupts the slot table).
-    val zoomState = rememberVicoZoomState(zoomEnabled = true, initialZoom = Zoom.Content)
+    // Key on layer count so Compose rebuilds the entire subtree when legend chip toggles
+    // add/remove layers. rememberCartesianChart uses vararg internally, so changing the
+    // argument count without a key corrupts the slot table.
+    key(layers.size) {
+        val zoomState = rememberVicoZoomState(zoomEnabled = true, initialZoom = Zoom.Content)

-    val markerVisibilityListener =
-        remember(onPointSelected) {
-            object : CartesianMarkerVisibilityListener {
-                override fun onShown(marker: CartesianMarker, targets: List<CartesianMarker.Target>) {
-                    targets.firstOrNull()?.let { onPointSelected?.invoke(it.x) }
-                }
+        val markerVisibilityListener =
+            remember(onPointSelected) {
+                object : CartesianMarkerVisibilityListener {
+                    override fun onShown(marker: CartesianMarker, targets: List<CartesianMarker.Target>) {
+                        targets.firstOrNull()?.let { onPointSelected?.invoke(it.x) }
+                    }

-                override fun onUpdated(marker: CartesianMarker, targets: List<CartesianMarker.Target>) {
-                    targets.firstOrNull()?.let { onPointSelected?.invoke(it.x) }
+                    override fun onUpdated(marker: CartesianMarker, targets: List<CartesianMarker.Target>) {
+                        targets.firstOrNull()?.let { onPointSelected?.invoke(it.x) }
+                    }
                }
            }
-        }

-    CartesianChartHost(
-        chart =
-        @Suppress("SpreadOperator")
-        rememberCartesianChart(
-            *layers.toTypedArray(),
-            startAxis = startAxis,
-            endAxis = endAxis,
-            bottomAxis = bottomAxis,
-            marker = marker,
-            markerVisibilityListener = markerVisibilityListener,
-            persistentMarkers = { _ -> if (selectedX != null && marker != null) marker at selectedX else null },
-            fadingEdges = rememberFadingEdges(),
-            decorations = decorations,
-        ),
-        modelProducer = modelProducer,
-        modifier = modifier,
-        scrollState = vicoScrollState,
-        zoomState = zoomState,
-    )
+        CartesianChartHost(
+            chart =
+            @Suppress("SpreadOperator")
+            rememberCartesianChart(
+                *layers.toTypedArray(),
+                startAxis = startAxis,
+                endAxis = endAxis,
+                bottomAxis = bottomAxis,
+                marker = marker,
+                markerVisibilityListener = markerVisibilityListener,
+                persistentMarkers = { _ -> if (selectedX != null && marker != null) marker at selectedX else null },
+                fadingEdges = rememberFadingEdges(),
+                decorations = decorations,
+                // Telemetry timestamps arrive at irregular intervals. Without an explicit
+                // x-step, Vico computes the GCD of consecutive x-value differences which can
+                // be as small as 1 second, making the chart logically enormous. A 60-second
+                // floor keeps the internal slot count reasonable for any practical interval.
+                getXStep = { model -> maxOf(model.getXDeltaGcd(), MIN_X_STEP_SECONDS) },
+            ),
+            modelProducer = modelProducer,
+            modifier = modifier,
+            scrollState = vicoScrollState,
+            zoomState = zoomState,
+        )
+    }
 }

 /**
--- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/ChartStyling.kt
+++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/ChartStyling.kt
@ -57,7 +57,7 @@ import com.patrykandpatrick.vico.compose.common.component.rememberTextComponent
 * **Design principles** (per [design#53](https://github.com/meshtastic/design/issues/53)):
 * - Default to thin lines **without** point markers to avoid clutter on dense timeseries.
 * - Show a single dot only at the marker/cursor position (handled by [rememberMarker]).
- * - Use `Interpolator.catmullRom()` for smooth curves that pass through every data point.
+ * - Use `Interpolator.cubic()` for smooth monotone curves that won't overshoot between sparse points.
 * - Reserve bold lines for the single most-important series; use subtle/gradient fills for secondary data.
 */
@Suppress("TooManyFunctions")
@ -73,15 +73,21 @@ object ChartStyling {
     *
     * @param lineColor The color of the line
     * @param lineWidth Width of the line in dp
+     * @param interpolator The line interpolation strategy. Defaults to monotone
+     *   [cubic][LineCartesianLayer.Interpolator.cubic] which won't overshoot between sparse data points (unlike
+     *   catmull-rom). Use [Sharp][LineCartesianLayer.Interpolator.Sharp] for discrete/integer metrics like hop counts.
     * @return Configured [LineCartesianLayer.Line]
     */
    @Composable
-    fun createStyledLine(lineColor: Color, lineWidth: Float = MEDIUM_LINE_WIDTH_DP): LineCartesianLayer.Line =
-        LineCartesianLayer.rememberLine(
-            fill = LineCartesianLayer.LineFill.single(Fill(lineColor)),
-            stroke = LineCartesianLayer.LineStroke.Continuous(lineWidth.dp),
-            interpolator = LineCartesianLayer.Interpolator.catmullRom(),
-        )
+    fun createStyledLine(
+        lineColor: Color,
+        lineWidth: Float = MEDIUM_LINE_WIDTH_DP,
+        interpolator: LineCartesianLayer.Interpolator = LineCartesianLayer.Interpolator.cubic(),
+    ): LineCartesianLayer.Line = LineCartesianLayer.rememberLine(
+        fill = LineCartesianLayer.LineFill.single(Fill(lineColor)),
+        stroke = LineCartesianLayer.LineStroke.Continuous(lineWidth.dp),
+        interpolator = interpolator,
+    )

    /**
     * Creates a line with a gradient area fill effect. Ideal for emphasising a single series or showing magnitude. The
@ -92,14 +98,18 @@ object ChartStyling {
     * @return Configured [LineCartesianLayer.Line]
     */
    @Composable
-    fun createGradientLine(lineColor: Color, lineWidth: Float = MEDIUM_LINE_WIDTH_DP): LineCartesianLayer.Line {
+    fun createGradientLine(
+        lineColor: Color,
+        lineWidth: Float = MEDIUM_LINE_WIDTH_DP,
+        interpolator: LineCartesianLayer.Interpolator = LineCartesianLayer.Interpolator.cubic(),
+    ): LineCartesianLayer.Line {
        val gradientBrush =
            Brush.verticalGradient(colors = listOf(lineColor.copy(alpha = 0.3f), lineColor.copy(alpha = 0.05f)))
        return LineCartesianLayer.rememberLine(
            fill = LineCartesianLayer.LineFill.single(Fill(lineColor)),
            areaFill = LineCartesianLayer.AreaFill.single(Fill(gradientBrush)),
            stroke = LineCartesianLayer.LineStroke.Continuous(lineWidth.dp),
-            interpolator = LineCartesianLayer.Interpolator.catmullRom(),
+            interpolator = interpolator,
        )
    }

@ -110,8 +120,11 @@ object ChartStyling {
     * @return Configured [LineCartesianLayer.Line]
     */
    @Composable
-    fun createBoldLine(lineColor: Color): LineCartesianLayer.Line =
-        createStyledLine(lineColor = lineColor, lineWidth = THICK_LINE_WIDTH_DP)
+    fun createBoldLine(
+        lineColor: Color,
+        interpolator: LineCartesianLayer.Interpolator = LineCartesianLayer.Interpolator.cubic(),
+    ): LineCartesianLayer.Line =
+        createStyledLine(lineColor = lineColor, lineWidth = THICK_LINE_WIDTH_DP, interpolator = interpolator)

    /**
     * Creates a subtle line suitable for secondary metrics that should not dominate the chart.
@ -131,7 +144,10 @@ object ChartStyling {
     * @return Configured [LineCartesianLayer.Line]
     */
    @Composable
-    fun createDashedLine(lineColor: Color): LineCartesianLayer.Line = LineCartesianLayer.rememberLine(
+    fun createDashedLine(
+        lineColor: Color,
+        interpolator: LineCartesianLayer.Interpolator = LineCartesianLayer.Interpolator.cubic(),
+    ): LineCartesianLayer.Line = LineCartesianLayer.rememberLine(
        fill = LineCartesianLayer.LineFill.single(Fill(lineColor)),
        stroke =
        LineCartesianLayer.LineStroke.Dashed(
@ -139,7 +155,7 @@ object ChartStyling {
            dashLength = 6.dp,
            gapLength = 3.dp,
        ),
-        interpolator = LineCartesianLayer.Interpolator.catmullRom(),
+        interpolator = interpolator,
    )

    /**
--- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/DeviceMetrics.kt
+++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/DeviceMetrics.kt
@ -307,12 +307,13 @@ private fun DeviceMetricsChart(
            }
        }

+        val percentRangeProvider = remember { CartesianLayerRangeProvider.fixed(minY = 0.0, maxY = 100.0) }
        val leftLayer =
            rememberConditionalLayer(
                hasData = leftLayerSeriesStyles.isNotEmpty(),
                lineProvider = LineCartesianLayer.LineProvider.series(leftLayerSeriesStyles),
                verticalAxisPosition = Axis.Position.Vertical.Start,
-                rangeProvider = CartesianLayerRangeProvider.fixed(minY = 0.0, maxY = 100.0),
+                rangeProvider = percentRangeProvider,
            )

        val rightLayer =
--- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentCharts.kt
+++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/EnvironmentCharts.kt
@ -158,11 +158,11 @@ fun EnvironmentMetricsChart(
                graphData.shouldPlot[(it.metricKey as? Environment)?.ordinal ?: 0]
            }

-        // Legend toggle state: tracks indices into allLegendData that are hidden
-        var hiddenIndices by remember { mutableStateOf(emptySet<Int>()) }
-        val hiddenMetrics =
-            remember(hiddenIndices, allLegendData) {
-                hiddenIndices.mapNotNull { allLegendData.getOrNull(it)?.metricKey as? Environment }.toSet()
+        // Track hidden metrics by key (not index) so toggling survives changes in allLegendData ordering.
+        var hiddenMetrics by remember { mutableStateOf(emptySet<Environment>()) }
+        val hiddenIndices =
+            remember(hiddenMetrics, allLegendData) {
+                allLegendData.indices.filter { (allLegendData[it].metricKey as? Environment) in hiddenMetrics }.toSet()
            }

        val colorToLabel = allLegendData.associate { it.color to (it.labelOverride ?: stringResource(it.nameRes)) }
@ -233,6 +233,7 @@ fun EnvironmentMetricsChart(
                },
            )

+        val pressureRangeProvider = remember { CartesianLayerRangeProvider.fixed(minY = 700.0, maxY = 1200.0) }
        val layers = mutableListOf<LineCartesianLayer>()
        if (showPressure && pressureData.isNotEmpty()) {
            layers.add(
@ -244,7 +245,7 @@ fun EnvironmentMetricsChart(
                    verticalAxisPosition = Axis.Position.Vertical.Start,
                    // Fixed range per Oscar's UX guidance: barometric pressure should NOT autoscale,
                    // otherwise trends (storms) are invisible. 700-1200 hPa covers sea-level to altitude.
-                    rangeProvider = CartesianLayerRangeProvider.fixed(minY = 700.0, maxY = 1200.0),
+                    rangeProvider = pressureRangeProvider,
                ),
            )
        }
@ -254,7 +255,7 @@ fun EnvironmentMetricsChart(
                when (metric) {
                    Environment.RADIATION,
                    Environment.WIND_SPEED,
-                    -> CartesianLayerRangeProvider.fixed(minY = 0.0)
+                    -> CartesianLayerRangeProvider.auto()
                    else -> null
                }
            val lineStyle =
@ -310,7 +311,8 @@ fun EnvironmentMetricsChart(
            modifier = Modifier.padding(top = 0.dp),
            hiddenSet = hiddenIndices,
            onToggle = { index ->
-                hiddenIndices = if (index in hiddenIndices) hiddenIndices - index else hiddenIndices + index
+                val metric = allLegendData.getOrNull(index)?.metricKey as? Environment ?: return@Legend
+                hiddenMetrics = if (metric in hiddenMetrics) hiddenMetrics - metric else hiddenMetrics + metric
            },
        )
    }
--- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PaxMetrics.kt
+++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/PaxMetrics.kt
@ -141,11 +141,20 @@ private fun PaxMetricsChart(
                rememberLineCartesianLayer(
                    lineProvider =
                    LineCartesianLayer.LineProvider.series(
-                        ChartStyling.createGradientLine(lineColor = bleColor),
-                        ChartStyling.createGradientLine(lineColor = wifiColor),
-                        ChartStyling.createBoldLine(lineColor = paxColor),
+                        ChartStyling.createGradientLine(
+                            lineColor = bleColor,
+                            interpolator = LineCartesianLayer.Interpolator.Sharp,
+                        ),
+                        ChartStyling.createGradientLine(
+                            lineColor = wifiColor,
+                            interpolator = LineCartesianLayer.Interpolator.Sharp,
+                        ),
+                        ChartStyling.createBoldLine(
+                            lineColor = paxColor,
+                            interpolator = LineCartesianLayer.Interpolator.Sharp,
+                        ),
                    ),
-                    rangeProvider = CartesianLayerRangeProvider.fixed(minY = 0.0),
+                    rangeProvider = CartesianLayerRangeProvider.auto(),
                ),
            ),
            startAxis = VerticalAxis.rememberStart(label = axisLabel),
--- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/TracerouteChart.kt
+++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/TracerouteChart.kt
@ -189,17 +189,26 @@ internal fun TracerouteMetricsChart(
        val forwardLayer =
            rememberConditionalLayer(
                hasData = forwardData.isNotEmpty(),
-                lineProvider = LineCartesianLayer.LineProvider.series(ChartStyling.createStyledLine(forwardColor)),
+                lineProvider =
+                LineCartesianLayer.LineProvider.series(
+                    ChartStyling.createStyledLine(
+                        forwardColor,
+                        interpolator = LineCartesianLayer.Interpolator.Sharp,
+                    ),
+                ),
                verticalAxisPosition = Axis.Position.Vertical.Start,
-                rangeProvider = CartesianLayerRangeProvider.fixed(minY = 0.0),
+                rangeProvider = CartesianLayerRangeProvider.auto(),
            )

        val returnLayer =
            rememberConditionalLayer(
                hasData = returnData.isNotEmpty(),
-                lineProvider = LineCartesianLayer.LineProvider.series(ChartStyling.createDashedLine(returnColor)),
+                lineProvider =
+                LineCartesianLayer.LineProvider.series(
+                    ChartStyling.createDashedLine(returnColor, interpolator = LineCartesianLayer.Interpolator.Sharp),
+                ),
                verticalAxisPosition = Axis.Position.Vertical.Start,
-                rangeProvider = CartesianLayerRangeProvider.fixed(minY = 0.0),
+                rangeProvider = CartesianLayerRangeProvider.auto(),
            )

        val rttLayer =