Fix PR comments

Author: wenli-cai

gradle/libs.versions.toml

@@ -51,7 +51,6 @@ google-material = "1.4.0"
 
 groovy = "3.0.9"
 jUnit = "4.13.2"
-junitJupiter = "5.10.1"
 java-diff-utils = "4.12"
 javaParser = "3.24.0"
 jetbrains-compose-plugin = "1.7.3"
@@ -206,7 +205,6 @@ java-diff-utils = { module = "io.github.java-diff-utils:java-diff-utils", versio
 jetbrains-annotations = "org.jetbrains:annotations:24.0.1"
 
 junit = { module = "junit:junit", version.ref = "jUnit" }
-junit-jupiter = { module = "org.junit.jupiter:junit-jupiter", version.ref = "junitJupiter" }
 
 kgx = { module = "com.rickbusarow.kgx:kotlin-gradle-extensions", version.ref = "kgx" }
 

workflow-trace-viewer/build.gradle.kts

@@ -31,7 +31,6 @@ kotlin {
       dependencies {
         implementation(kotlin("test"))
         implementation(kotlin("test-junit5"))
-        implementation(libs.junit.jupiter)
       }
     }
   }

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

@@ -9,11 +9,12 @@ import androidx.compose.runtime.mutableIntStateOf
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.setValue
+import androidx.compose.runtime.snapshotFlow
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.geometry.Offset
 import com.squareup.workflow1.traceviewer.model.Node
-import com.squareup.workflow1.traceviewer.model.NodeDiff
+import com.squareup.workflow1.traceviewer.model.NodeUpdate
 import com.squareup.workflow1.traceviewer.ui.FrameSelectTab
 import com.squareup.workflow1.traceviewer.ui.RenderDiagram
 import com.squareup.workflow1.traceviewer.ui.RightInfoPanel
@@ -29,13 +30,15 @@ public fun App(
   modifier: Modifier = Modifier
 ) {
   var selectedTraceFile by remember { mutableStateOf<PlatformFile?>(null) }
-  var selectedNode by remember { mutableStateOf<NodeDiff?>(null) }
+  var selectedNode by remember { mutableStateOf<NodeUpdate?>(null) }
   var workflowFrames by remember { mutableStateOf<List<Node>>(emptyList()) }
   var frameIndex by remember { mutableIntStateOf(0) }
   val sandboxState = remember { SandboxState() }
 
-  LaunchedEffect(frameIndex) {
-    sandboxState.reset()
+  LaunchedEffect(sandboxState) {
+    snapshotFlow { frameIndex }.collect {
+      sandboxState.reset()
+    }
   }
 
   Box(
@@ -51,7 +54,7 @@ public fun App(
           frameInd = frameIndex,
           onFileParse = { workflowFrames = it },
           onNodeSelect = { node, prevNode ->
-            selectedNode = NodeDiff(node, prevNode)
+            selectedNode = NodeUpdate(node, prevNode)
           }
         )
       }

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

@@ -1,5 +1,7 @@
 package com.squareup.workflow1.traceviewer.model
 
+import com.squareup.moshi.Json
+
 /**
  * Since the logic of Workflow is hierarchical (where each workflow may have parent workflows and/or
  * children workflows, a tree structure is most appropriate for representing the data rather than
@@ -15,7 +17,7 @@ internal data class Node(
   val props: String,
   val state: String,
   val rendering: String = "",
-  val children: List<Node>,
+  @Transient val children: LinkedHashMap<String, Node> = LinkedHashMap()
 ) {
 
   override fun toString(): String {
@@ -50,9 +52,9 @@ internal data class Node(
 }
 
 internal fun Node.addChild(child: Node): Node {
-  return copy(children = this.children + child)
+  return copy(children = (this.children.plus(child.id to child) as LinkedHashMap<String, Node>))
 }
 
 internal fun Node.replaceChild(child: Node): Node {
-  return copy(children = this.children.map { if (it.id == child.id) child else it })
+  return copy(children = (this.children.plus(child.id to child) as LinkedHashMap<String, Node>))
 }

workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/model/NodeDiff.kt renamed to workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/model/NodeUpdate.kt

@@ -6,7 +6,7 @@ package com.squareup.workflow1.traceviewer.model
  *
  * If it's the first node in the frame, [previous] will be null and there is no difference to show.
  */
-internal class NodeDiff(
+internal class NodeUpdate(
   val current: Node,
   val previous: Node?,
 )

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

@@ -34,7 +34,7 @@ import androidx.compose.ui.text.font.FontStyle
 import androidx.compose.ui.text.font.FontWeight
 import androidx.compose.ui.unit.dp
 import com.squareup.workflow1.traceviewer.model.Node
-import com.squareup.workflow1.traceviewer.model.NodeDiff
+import com.squareup.workflow1.traceviewer.model.NodeUpdate
 
 /**
  * A panel that displays information about the selected workflow node.
@@ -44,7 +44,7 @@ import com.squareup.workflow1.traceviewer.model.NodeDiff
  */
 @Composable
 internal fun RightInfoPanel(
-  selectedNode: NodeDiff?,
+  selectedNode: NodeUpdate?,
   modifier: Modifier = Modifier
 ) {
   Row(
@@ -80,7 +80,7 @@ internal fun RightInfoPanel(
  */
 @Composable
 private fun NodePanelDetails(
-  node: NodeDiff?,
+  node: NodeUpdate?,
   modifier: Modifier = Modifier
 ) {
   LazyColumn(

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

@@ -13,9 +13,11 @@ import androidx.compose.material.Text
 import androidx.compose.runtime.Composable
 import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.runtime.getValue
+import androidx.compose.runtime.mutableStateListOf
 import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.setValue
+import androidx.compose.runtime.snapshots.SnapshotStateList
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.graphics.Color
@@ -39,9 +41,9 @@ internal fun RenderDiagram(
 ) {
   var isLoading by remember(traceFile) { mutableStateOf(true) }
   var error by remember(traceFile) { mutableStateOf<Throwable?>(null) }
-  var frames by remember { mutableStateOf<List<Node>>(emptyList()) }
-  var fullTree by remember { mutableStateOf<List<Node>>(emptyList()) }
-  var affectedNodes by remember { mutableStateOf<List<Set<Node>>>(emptyList()) }
+  var frames = remember { mutableStateListOf<Node>() }
+  var fullTree = remember { mutableStateListOf<Node>() }
+  var affectedNodes = remember { mutableStateListOf<Set<Node>>() }
 
   LaunchedEffect(traceFile) {
     val parseResult = parseTrace(traceFile)
@@ -52,9 +54,9 @@ internal fun RenderDiagram(
       }
       is ParseResult.Success -> {
         val parsedFrames = parseResult.trace ?: emptyList()
-        frames = parsedFrames
-        fullTree = parseResult.trees
-        affectedNodes = parseResult.affectedNodes
+        frames.addAll(parsedFrames)
+        fullTree.addAll(parseResult.trees)
+        affectedNodes.addAll(parseResult.affectedNodes)
         onFileParse(parsedFrames)
         isLoading = false
       }
@@ -104,8 +106,8 @@ private fun DrawTree(
         In the edge case that the current frame has additional children compared to the previous
         frame, we replace with null and will check before next recursive call.
        */
-      node.children.forEachIndexed { index, childNode ->
-        val prevChildNode = previousNode?.children?.getOrNull(index)
+      node.children.forEach { (index, childNode) ->
+        val prevChildNode = previousNode?.children?.get(index)
         DrawTree(childNode, prevChildNode, affectedNodes, onNodeSelect)
       }
     }

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

@@ -9,6 +9,7 @@ import com.squareup.workflow1.traceviewer.model.addChild
 import com.squareup.workflow1.traceviewer.model.replaceChild
 import io.github.vinceglb.filekit.PlatformFile
 import io.github.vinceglb.filekit.readString
+import java.util.LinkedHashMap
 
 /*
  The root workflow Node uses an ID of 0, and since we are filtering childrenByParent by the
@@ -77,14 +78,15 @@ private fun getFrameFromRenderPass(renderPass: List<Node>): Node {
  */
 private fun buildTree(node: Node, childrenByParent: Map<String, List<Node>>): Node {
   val children = (childrenByParent[node.id] ?: emptyList())
+    .map { buildTree(it, childrenByParent) }
   return Node(
     name = node.name,
     id = node.id,
     parent = node.parent,
     parentId = node.parentId,
     props = node.props,
     state = node.state,
-    children = children.map { buildTree(it, childrenByParent) },
+    children = LinkedHashMap(children.associateBy { it.id }),
   )
 }
 
@@ -99,14 +101,12 @@ internal fun mergeFrameIntoMainTree(
   frame: Node,
   main: Node
 ): Node {
-  if (frame.id != main.id) {
-    throw IllegalArgumentException("Frame root ID does not match main tree root ID.")
-  }
+  require(frame.id == main.id)
 
   val updatedNode = frame.copy(children = main.children)
 
-  return frame.children.fold(updatedNode) { mergedTree, frameChild ->
-    val mainTreeChild = mergedTree.children.singleOrNull { it.id == frameChild.id }
+  return frame.children.values.fold(updatedNode) { mergedTree, frameChild ->
+    val mainTreeChild = mergedTree.children[frameChild.id]
     if (mainTreeChild != null) {
       mergedTree.replaceChild(mergeFrameIntoMainTree(frameChild, mainTreeChild))
     } else {

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

@@ -42,8 +42,13 @@ internal fun SandboxBackground(
           val event = awaitPointerEvent()
           if (event.type == PointerEventType.Scroll) {
             val scrollDelta = event.changes.first().scrollDelta.y
+            // Applies zoom factor based on the actual delta change rather than just the act of scrolling
+            // This helps to normalize mouse scrolling and touchpad scrolling, since touchpad will
+            // fire a lot more scroll events.
             val factor = 1f + (-scrollDelta * 0.1f)
-            sandboxState.scale = (sandboxState.scale * factor).coerceIn(0.1f, 10f)
+            val minWindowSize = 0.1f
+            val maxWindowSize = 10f
+            sandboxState.scale = (sandboxState.scale * factor).coerceIn(minWindowSize, maxWindowSize)
             event.changes.forEach { it.consume() }
           }
         }