Fix PR comments

Author: wenli-cai

workflow-trace-viewer/api/workflow-trace-viewer.api

@@ -33,7 +33,3 @@ public final class com/squareup/workflow1/traceviewer/ui/control/ComposableSingl
 	public final fun getLambda-1$wf1_workflow_trace_viewer ()Lkotlin/jvm/functions/Function3;
 }
 
-public final class com/squareup/workflow1/traceviewer/util/parser/JsonParserKt {
-	public static final field ROOT_ID Ljava/lang/String;
-}
-

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/App.kt

@@ -19,8 +19,10 @@ import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.geometry.Offset
 import androidx.compose.ui.layout.onGloballyPositioned
+import androidx.compose.ui.layout.onSizeChanged
 import androidx.compose.ui.unit.IntSize
 import androidx.compose.ui.unit.dp
+import androidx.compose.ui.window.WindowPosition.PlatformDefault.x
 import com.squareup.workflow1.traceviewer.model.Node
 import com.squareup.workflow1.traceviewer.model.NodeUpdate
 import com.squareup.workflow1.traceviewer.ui.RightInfoPanel
@@ -63,8 +65,8 @@ internal fun App(
   }
 
   Box(
-    modifier = modifier.onGloballyPositioned {
-      appWindowSize = it.size
+    modifier = modifier.onSizeChanged {
+      appWindowSize = it
     }
   ) {
     fun resetStates() {
@@ -113,17 +115,20 @@ internal fun App(
           // frameSize has not been updated yet, so on the first frame, frameSize = nodeLocations.size = 0,
           // and it will append a new map
           while (nodeLocations.size <= frameSize) {
-            nodeLocations.add(mutableStateMapOf())
+            nodeLocations += mutableStateMapOf()
           }
         }
 
+        val frameNodeLocations = nodeLocations[frameInd]
         SearchBox(
-          nodes = nodeLocations[frameInd].keys.toList(),
+          nodes = frameNodeLocations.keys.toList(),
           onSearch = { name ->
             sandboxState.scale = 1f
-            val node = nodeLocations[frameInd].keys.firstOrNull { it.name == name }
-            val newX = sandboxState.offset.x - nodeLocations[frameInd][node]!!.x + appWindowSize.width / 2
-            val newY = sandboxState.offset.y - nodeLocations[frameInd][node]!!.y + appWindowSize.height / 2
+            val node = frameNodeLocations.keys.first { it.name == name }
+            val newX = (sandboxState.offset.x - frameNodeLocations.getValue(node).x
+              + appWindowSize.width / 2)
+            val newY = (sandboxState.offset.y - frameNodeLocations.getValue(node).y
+              + appWindowSize.height / 2)
             sandboxState.offset = Offset(x = newX, y = newY)
           },
         )
@@ -216,7 +221,7 @@ internal sealed interface TraceMode {
 /**
  * Allows users to select from multiple devices that are currently running.
  */
-internal fun listDevices(): List<String> {
+private fun listDevices(): List<String> {
   val process = ProcessBuilder("adb", "devices", "-l").start()
   process.waitFor()
   // We drop the header "List of devices attached"

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/model/Node.kt

@@ -33,17 +33,15 @@ internal data class Node(
   }
 
   companion object {
-    fun getNodeFields(): List<String> {
-      return listOf("Props", "State")
-    }
-
-    fun getNodeData(node: Node, field: String): String {
-      return when (field.lowercase()) {
-        "props" -> node.props
-        "state" -> node.state
-        else -> throw IllegalArgumentException("Unknown field: $field")
-      }
-    }
+    val nodeFields: List<String> = listOf("Props", "State")
+  }
+}
+
+internal fun Node.getNodeData(field: String): String {
+  return when (field.lowercase()) {
+    "props" -> props
+    "state" -> state
+    else -> throw IllegalArgumentException("Unknown field: $field")
   }
 }
 

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/ui/WorkflowInfoPanel.kt

@@ -36,6 +36,7 @@ import androidx.compose.ui.text.style.TextOverflow
 import androidx.compose.ui.unit.dp
 import com.squareup.workflow1.traceviewer.model.Node
 import com.squareup.workflow1.traceviewer.model.NodeUpdate
+import com.squareup.workflow1.traceviewer.model.getNodeData
 import com.squareup.workflow1.traceviewer.util.parser.computeAnnotatedDiff
 
 /**
@@ -123,10 +124,10 @@ private fun NodePanelDetails(
       )
     }
 
-    val fields = Node.getNodeFields()
+    val fields = Node.nodeFields
     for (field in fields) {
-      val currVal = Node.getNodeData(node.current, field)
-      val pastVal = if (node.past != null) Node.getNodeData(node.past, field) else null
+      val currVal = node.current.getNodeData(field)
+      val pastVal = if (node.past != null) node.past.getNodeData(field) else null
       item {
         DetailCard(
           label = field,

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/ui/WorkflowTree.kt

@@ -13,9 +13,11 @@ import androidx.compose.foundation.layout.Row
 import androidx.compose.foundation.layout.fillMaxSize
 import androidx.compose.foundation.layout.fillMaxWidth
 import androidx.compose.foundation.layout.padding
+import androidx.compose.foundation.layout.wrapContentSize
 import androidx.compose.foundation.shape.RoundedCornerShape
 import androidx.compose.material.Text
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.DisposableEffect
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
 import androidx.compose.runtime.remember
@@ -75,7 +77,7 @@ internal fun DrawTree(
       isAffected = isAffected,
       isExpanded = isExpanded,
       onNodeSelect = onNodeSelect,
-      onExpandToggle = { expandedNodes[node.id] = !expandedNodes[node.id]!! },
+      onExpandToggle = { expandedNodes[node.id] = !expandedNodes.getValue(node.id) },
       storeNodeLocation = storeNodeLocation
     )
 
@@ -126,8 +128,9 @@ private fun UnaffectedChildrenGroup(
   if (children.isEmpty()) return
 
   val groupKey = "${node.id}_unaffected_group"
-  LaunchedEffect(Unit) {
+  DisposableEffect(Unit) {
     expandedNodes[groupKey] = false
+    onDispose {}
   }
   val isGroupExpanded = expandedNodes[groupKey] == true
 
@@ -325,7 +328,10 @@ private fun DrawNode(
           storeNodeLocation(node, offsetToCenter)
         }
     ) {
-      Column(horizontalAlignment = Alignment.CenterHorizontally) {
+      Column(
+        horizontalAlignment = Alignment.CenterHorizontally,
+        modifier = Modifier.wrapContentSize()
+      ) {
         Row(
           verticalAlignment = Alignment.CenterVertically,
           horizontalArrangement = Arrangement.spacedBy(4.dp)
@@ -358,16 +364,14 @@ private fun NodeTooltip(
   nodeState: NodeState,
   modifier: Modifier = Modifier
 ) {
-  Box(
+  Text(
     modifier = modifier
+      .wrapContentSize()
       .clip(RoundedCornerShape(4.dp))
       .background(Color.Black.copy(alpha = 0.3f))
-      .padding(horizontal = 8.dp, vertical = 4.dp)
-  ) {
-    Text(
-      text = nodeState.name,
-      color = Color.White,
-      fontSize = 12.sp
-    )
-  }
+      .padding(horizontal = 8.dp, vertical = 4.dp),
+    text = nodeState.name,
+    color = Color.White,
+    fontSize = 12.sp
+  )
 }

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/ui/control/DisplayDevices.kt

@@ -10,11 +10,17 @@ import androidx.compose.material.Card
 import androidx.compose.material.ExperimentalMaterialApi
 import androidx.compose.material.Text
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.key
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.graphics.Color
 import androidx.compose.ui.unit.dp
 
+/**
+ * Only give back the specific emulator device, i.e. "emulator-5554"
+ */
+private val emulatorRegex = Regex("""\bemulator-\d+\b""")
+
 @OptIn(ExperimentalMaterialApi::class)
 @Composable
 internal fun DisplayDevices(
@@ -35,25 +41,25 @@ internal fun DisplayDevices(
       return@Box
     }
 
-    val emulatorRegex = Regex("""\bemulator-\d+\b""")
     Column {
       devices.forEach { device ->
-        Card(
-          onClick = {
-            // Only give back the specific emulator device, i.e. "emulator-5554"
-            emulatorRegex.find(device)?.value?.let { emulator ->
-              onDeviceSelect(emulator)
-            }
-          },
-          shape = RoundedCornerShape(16.dp),
-          border = BorderStroke(1.dp, Color.Gray),
-          modifier = Modifier.padding(4.dp),
-          elevation = 2.dp
-        ) {
-          Text(
-            text = device,
-            modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp)
-          )
+        key(device) {
+          Card(
+            onClick = {
+              emulatorRegex.find(device)?.value?.let { emulator ->
+                onDeviceSelect(emulator)
+              }
+            },
+            shape = RoundedCornerShape(16.dp),
+            border = BorderStroke(1.dp, Color.Gray),
+            modifier = Modifier.padding(4.dp),
+            elevation = 2.dp
+          ) {
+            Text(
+              text = device,
+              modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp)
+            )
+          }
         }
       }
     }

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/ui/control/SearchBox.kt

@@ -15,6 +15,7 @@ import androidx.compose.material3.SearchBarColors
 import androidx.compose.material3.SearchBarDefaults
 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.setValue
@@ -65,18 +66,20 @@ internal fun SearchBox(
     val relevantNodes = nodes.filter { it.name.contains(searchText, ignoreCase = true) }
     Column {
       relevantNodes.take(5).forEach { node ->
-        ListItem(
-          headlineContent = { Text(node.name) },
-          modifier = Modifier
-            .clickable {
-              onSearch(node.name)
-              expanded = false
-            },
-          colors = ListItemDefaults.colors(
-            containerColor = Color.White,
-            headlineColor = Color.Black
+        key(node.id) {
+          ListItem(
+            headlineContent = { Text(node.name) },
+            modifier = Modifier
+              .clickable {
+                onSearch(node.name)
+                expanded = false
+              },
+            colors = ListItemDefaults.colors(
+              containerColor = Color.White,
+              headlineColor = Color.Black
+            )
           )
-        )
+        }
       }
     }
   }

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/util/SandboxBackground.kt

@@ -38,7 +38,7 @@ internal fun SandboxBackground(
           sandboxState.offset += translation
         }
       }
-      .pointerInput(Unit) {
+      .pointerInput(appWindowSize) {
         // Zooming capabilities: watches for any scroll events and immediately consumes changes.
         // - This is AI generated.
         awaitEachGesture {

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/util/parser/DiffUtils.kt

@@ -8,6 +8,12 @@ import com.github.difflib.text.DiffRow.Tag
 import com.github.difflib.text.DiffRowGenerator
 import com.squareup.workflow1.traceviewer.util.parser.DiffStyles.buildStringWithStyle
 
+/**
+ * Matching for the type name of a field. This is used to pull out the name to compare
+ * Loading() -> Loading; Idle() -> Idle; CheckoutAppletWorkflow() -> CheckoutAppletWorkflow
+ */
+private val stateRegex = Regex("""^(\w+)\(""")
+
 /**
  * Generates a field-level word-diff for each node's states.
  *
@@ -116,35 +122,36 @@ internal fun computeAnnotatedDiff(
  * Parses the full diff within Tag.CHANGED to give back a list of operations to perform
  */
 private fun parseChangedDiff(fullDiff: String): List<Pair<SpanStyle, String>> {
-  val operations: MutableList<Pair<SpanStyle, String>> = mutableListOf()
-  var i = 0
-  while (i < fullDiff.length) {
-    when {
-      fullDiff.startsWith("--", i) -> {
-        val end = fullDiff.indexOf("--", i + 2)
-        if (end != -1) {
-          val removed = fullDiff.substring(i + 2, end)
-          operations.add(DiffStyles.DELETE to removed)
-          i = end + 2
+  val operations = buildList {
+    var i = 0
+    while (i < fullDiff.length) {
+      when {
+        fullDiff.startsWith("--", i) -> {
+          val end = fullDiff.indexOf("--", i + 2)
+          if (end != -1) {
+            val removed = fullDiff.substring(i + 2, end)
+            add(DiffStyles.DELETE to removed)
+            i = end + 2
+          }
         }
-      }
 
-      fullDiff.startsWith("++", i) -> {
-        val end = fullDiff.indexOf("++", i + 2)
-        if (end != -1) {
-          val added = fullDiff.substring(i + 2, end)
-          operations.add(DiffStyles.INSERT to added)
-          i = end + 2
+        fullDiff.startsWith("++", i) -> {
+          val end = fullDiff.indexOf("++", i + 2)
+          if (end != -1) {
+            val added = fullDiff.substring(i + 2, end)
+            add(DiffStyles.INSERT to added)
+            i = end + 2
+          }
         }
-      }
 
-      else -> {
-        val nextTagStart = listOf(
-          fullDiff.indexOf("--", i),
-          fullDiff.indexOf("++", i)
-        ).filter { it >= 0 }.minOrNull() ?: fullDiff.length
-        operations.add(DiffStyles.UNCHANGED to fullDiff.substring(i, nextTagStart))
-        i = nextTagStart
+        else -> {
+          val nextTagStart = listOf(
+            fullDiff.indexOf("--", i),
+            fullDiff.indexOf("++", i)
+          ).filter { it >= 0 }.minOrNull() ?: fullDiff.length
+          add(DiffStyles.UNCHANGED to fullDiff.substring(i, nextTagStart))
+          i = nextTagStart
+        }
       }
     }
   }
@@ -216,7 +223,6 @@ private fun getFieldsAsList(field: String): List<String> {
 }
 
 private fun extractTypeName(field: String): String {
-  val stateRegex = Regex("""^(\w+)\(""")
   // If regex doesn't match, that means it's likely "kotlin.Unit" or "0"
   return stateRegex.find(field)?.groupValues?.get(1) ?: field
 }