Workflow visualizer UI improvement #1366

wenli-cai · 2025-07-10T14:20:54Z

UI changes

The specific nodes that were affected in the render pass now highlighted in context to the whole tree, rather than just showing the specific frame. This makes it easier to see the exact process and layers of workflow that were passed through
UI for displaying node data is improved. It also shows the transition between the node's previous state and current state to allow for better understanding on what actually changed.
The display window is also recentered each time a new frame is selected.

Code changes

More fields were added to Node to support visualization capabilities, and is closely related with how ActionLogger records data from workflow runtime.
NodeDiff was created to store a node's previous and current state.
All UI functions are now internal, since there's no need to support any consumers using this specific API (as it is meant to be run as a downloadable application).

Hoists the UI state up to App.kt to allow for manual reset each time a different frame is selected

1. Force it to consume all pointerInput, meaning there cannot be interactino with any objects shadowed by it. 2. Hoist alignment into App.kt instead of forcing it to the right with a full Spacer

Currently this builds one tree which is an amalgamation of all the frames. It uses a recursive algorithm to fill in missing children nodes after each renderpass has been parsed

These can be viewed as pairs: for each frame, there is the full workflow tree that is slowly built up with the new nodes. Add a copy method onto Node class to store a current copy of the workflow tree.

This commit introduces the idea of a NodeDiff, which stores the node's current and previous state - from the previous frame - in order to better represent what is actually changing through the UI. If there is no previous frame (only on the first frame of the trace), there is no need to show a diff, so we plainly depict the node's states.

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

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

steve-the-edwards · 2025-07-14T14:38:02Z

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

+}
+
+internal fun Node.replaceChild(child: Node): Node {
+  return copy(children = this.children.map { if (it.id == child.id) child else it })


immutable maps can use the plus operator (.plus or + as infix) to do this without having to map each node. e.g.,

Suggested change

return copy(children = this.children.map { if (it.id == child.id) child else it })

return copy(children = this.children.plus(child.id to child))

That will replace only the value with child.id as its key.

Kotlin Playground link: https://pl.kotl.in/8IX2XBth3

.plus would work on a map, but children is a list of Nodes. Would you think a map is a better use case here? I was thinking I could at least keep the insertion order using a List, unless I used something like a LinkedHashMap?

Sounds like LinkedHashMap would be better for this case.

Sounds good, doing this would also mean that addChild and replaceChild are the exact same in implementation. Is it good still keep those as different functions, though, to make the recursive algorithm easier to understand?

you can leave them separate as long as one uses the other (so you avoid having to update multiple places with the duplicate code).

WEre you going to do this? Or on a later PR?

Fixed in another comment here

steve-the-edwards · 2025-07-14T14:40:12Z

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

+package com.squareup.workflow1.traceviewer.model
+
+/**
+ * Represents the difference between the current and previous state of a node in the workflow trace.


It does not really represent the difference in any way. It just holds both nodes.

It might better be called NodeUpdate or something like that?

You might start adding some diffing methods on this class to help with that (if it's useful).

That's true, NodeUpdate is good.

steve-the-edwards · 2025-07-14T14:41:34Z

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

+        modifier = Modifier.padding(top = 8.dp, bottom = 8.dp)
+      )
+    }
+    val fields = listOf("Name", "Id", "Props", "State", "Rendering")


If you stick with this method (which I think is fine btw!) it would be good to just have this list as a constant on Node so that it keeps it close the code its representing.

Fixed with reflection in this PR #1367

steve-the-edwards · 2025-07-14T14:57:08Z

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

+    if (mainTreeChild != null) {
+      mergedTree.replaceChild(mergeFrameIntoMainTree(frameChild, mainTreeChild))
+    } else {
+      mergedTree.addChild(frameChild)
+    }


Another option rather than keeping a separate list of affectedNodes would be to mark up these nodes as they are parsed within the actual node class itself. I.e. you could have an enum of change types and a field on Node for status., e.g., status is SAME, NEW, PROPS_CHANGED, STATE_CHANGED, CHILD_STATE_CHANGED or something like that? That way we can continue to add more colouring information to the UI for different aspects.

Yeah this would be great to have. I'll note this as a TODO in the future.

steve-the-edwards · 2025-07-14T15:01:30Z

workflow-trace-viewer/build.gradle.kts

+      dependencies {
+        implementation(kotlin("test"))
+        implementation(kotlin("test-junit5"))
+        implementation(libs.junit.jupiter)


What is jupiter for? I could not see something related to this on your unit test that you wrote?

It wasn't being used for anything, thanks for catching that. I had goose write the tests and it had just included it. Is it cleaning if it's removed?

Would be great to remove this if it's not needed.

zach-klippenstein · 2025-07-14T17:34:48Z

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

+  LaunchedEffect(frameIndex) {
+    sandboxState.reset()
+  }


This is an anti-pattern (written about multiple times in Compose Chronicles).

Should be

LaunchedEffect(sandboxState) { snapshotFlow { frameIndex }.collect { sandboxState.reset() } }

zach-klippenstein · 2025-07-14T17:38:41Z

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

+}
+
+internal fun Node.replaceChild(child: Node): Node {
+  return copy(children = this.children.map { if (it.id == child.id) child else it })


Sounds like LinkedHashMap would be better for this case.

zach-klippenstein · 2025-07-14T17:40:11Z

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

+  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()) }


Nit: Can any of these be SnapshotStateLists?

Or if you want them to be immutable, then consider using ImmutableList and friends.

I didn't know about this, this is great to know! The next step in the project would require these being mutable, so thanks for pointing this out

zach-klippenstein · 2025-07-14T17:42:28Z

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

+  if (frame.id != main.id) {
+    throw IllegalArgumentException("Frame root ID does not match main tree root ID.")
+  }


Nit: Could be require

zach-klippenstein · 2025-07-14T17:43:04Z

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

+            val factor = 1f + (-scrollDelta * 0.1f)
+            sandboxState.scale = (sandboxState.scale * factor).coerceIn(0.1f, 10f)


Can you add a comment explaining what this math is for and what the constants mean?

zach-klippenstein · 2025-07-14T17:44:59Z

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

+      if (!open) {
+        return@Card
+      }


UX nit: Would AnimatedVisibility look more polished?

Definitely, there's a lot of ui/ux changes I have written down to look more polished, and it's something I'm looking to complete later in the project.

zach-klippenstein · 2025-07-14T17:49:14Z

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

+  val state: String,
+  val rendering: String = "",
  val children: List<Node>,
 ) {


This class looks like it's intended to basically be a database table, where the properties are referenced dynamically by the UI. Can these properties just be stored in a map, with keys defined as constants? No need to use reflection.

These properties are all here for the purpose of moshi being able to parse a file into these objects. Unless there is a way to use moshi in the way you're describing it.

Even then, I'm not sure what the difference is. Could you help me explain the use case of having a data class like this vs using a map and treating it like a database table?

The map woudl be just key : value where both are strings. Since everything but children here is a string, the idea would be that essentially we are using this data class to store a bunch of String properties. Since they are all strings, we could just make it a nodeProperties = mapOf<String, String>() for everything but the children. once that is a map then you are able to easily iterate both the 'properties' keys (with nodeProperties.keys.) and the values (with nodeProperties.values) without needing to use reflection etc.

As for Moshi I imagine they have a default adapter that just maps JSON properties to a map.

Where it gets slightly more complicated is with the children property here, since that is not a String and is why we have this as a full class.

You could do something like:

data class Node ( val properties: ImmutableMap<String, String>, val children: LinkedHashMap<String, Node>) {}

and then you'd have to change the parsing. ultimately probably that would be the 'best' fit for the data (and you'd be able to avoid using reflection as the properties list change since you'd always iterate over all the keys).

As for whether you do that now, I'm not too worried either way 🤷🏻

understood, this makes sense. However, I still don't understand the distinction that @zach-klippenstein made originally about classes and "database table classes". Are data classes not meant to be used in a way that just holds data?

ya they are meant for that. I think the distinction comes down to the type of data. if you have properties of different strongly types vs if you just have a set of String properties.

ah gotcha, I understand now, thanks for clarifying that.

zach-klippenstein · 2025-07-14T17:51:24Z

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

  Box(
    modifier = modifier
  ) {


Not in PR scope, but you probably want propagateMinConstraints here.

Could you explain this more? As far as I understand, this would be telling that all children containers need to be a specific size within the Box?

japplin · 2025-07-14T20:32:03Z

workflow-trace-viewer/build.gradle.kts

  }
 }
+
+tasks.named<Test>("jvmTest") {


I also wonder if this is not needed?

+1 - do we know why this is needed? if not, lets remove.

it seems like it's needed for registering and running junit tests in a multiplatform app?
https://kotlinlang.org/docs/jvm-test-using-junit.html#add-dependencies

But I havent removed the logging parts afterwards, those aren't necessary.

Great. sounds good.

steve-the-edwards · 2025-07-18T15:04:07Z

workflow-trace-viewer/build.gradle.kts

  }
 }
+
+tasks.named<Test>("jvmTest") {


+1 - do we know why this is needed? if not, lets remove.

steve-the-edwards · 2025-07-18T15:13:22Z

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

+  val state: String,
+  val rendering: String = "",
  val children: List<Node>,
 ) {


The map woudl be just key : value where both are strings. Since everything but children here is a string, the idea would be that essentially we are using this data class to store a bunch of String properties. Since they are all strings, we could just make it a nodeProperties = mapOf<String, String>() for everything but the children. once that is a map then you are able to easily iterate both the 'properties' keys (with nodeProperties.keys.) and the values (with nodeProperties.values) without needing to use reflection etc.

As for Moshi I imagine they have a default adapter that just maps JSON properties to a map.

Where it gets slightly more complicated is with the children property here, since that is not a String and is why we have this as a full class.

You could do something like:

data class Node ( val properties: ImmutableMap<String, String>, val children: LinkedHashMap<String, Node>) {}

and then you'd have to change the parsing. ultimately probably that would be the 'best' fit for the data (and you'd be able to avoid using reflection as the properties list change since you'd always iterate over all the keys).

As for whether you do that now, I'm not too worried either way 🤷🏻

steve-the-edwards · 2025-07-18T15:15:56Z

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

+}
+
+internal fun Node.replaceChild(child: Node): Node {
+  return copy(children = this.children.map { if (it.id == child.id) child else it })


you can leave them separate as long as one uses the other (so you avoid having to update multiple places with the duplicate code).

steve-the-edwards · 2025-07-18T15:16:36Z

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

+}
+
+internal fun Node.replaceChild(child: Node): Node {
+  return copy(children = this.children.map { if (it.id == child.id) child else it })


WEre you going to do this? Or on a later PR?

steve-the-edwards · 2025-07-18T15:21:01Z

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

+}
+
+internal fun Node.replaceChild(child: Node): Node {
+  return copy(children = LinkedHashMap(this.children.plus(child.id to child)))


Make replaceChild call addChild so you only have 1 place to update/maintain

wenli-cai added 9 commits July 10, 2025 10:30

Use wrapper around sandbox visual states

f946bcf

Hoists the UI state up to App.kt to allow for manual reset each time a different frame is selected

Change sidebar functionality

7285e68

1. Force it to consume all pointerInput, meaning there cannot be interactino with any objects shadowed by it. 2. Hoist alignment into App.kt instead of forcing it to the right with a full Spacer

Build one main workflow runtime tree

7070d83

Currently this builds one tree which is an amalgamation of all the frames. It uses a recursive algorithm to fill in missing children nodes after each renderpass has been parsed

Normalize scroll zoom sensitivity for mouse vs touchpad

c6264da

Include a list of trees that pairs with parsed frames

193901d

These can be viewed as pairs: for each frame, there is the full workflow tree that is slowly built up with the new nodes. Add a copy method onto Node class to store a current copy of the workflow tree.

WIP

a262f7b

Generate full tree diagrams

ef6ba80

Highlight each render pass detail within the full tree

c243292

Change junit dependency to library catalog

7481f36

wenli-cai force-pushed the wenli/visualizer-ui-improvements branch from 2c50fde to 370d1c7 Compare July 10, 2025 14:48

Clean up comments and fix build errors

13fa318

wenli-cai force-pushed the wenli/visualizer-ui-improvements branch from cf2c3ed to 13fa318 Compare July 10, 2025 15:04

Fix compose lint violations

8f5514d

wenli-cai force-pushed the wenli/visualizer-ui-improvements branch from a5ea5dd to b989a1a Compare July 11, 2025 17:53

wenli-cai added 2 commits July 11, 2025 14:06

Clean up

69d9c27

Improve UI for detailed node panel

32c5b9f

wenli-cai force-pushed the wenli/visualizer-ui-improvements branch 2 times, most recently from 5ebf042 to 32c5b9f Compare July 11, 2025 20:27

wenli-cai marked this pull request as ready for review July 11, 2025 20:35

wenli-cai requested review from a team and zach-klippenstein as code owners July 11, 2025 20:35

wenli-cai commented Jul 11, 2025

View reviewed changes

...w-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/ui/WorkflowInfoPanel.kt Show resolved Hide resolved

Fix compose lint violations

abe0486

wenli-cai force-pushed the wenli/visualizer-ui-improvements branch from 471f276 to abe0486 Compare July 11, 2025 20:46

Apply changes from apiDump

841fee2

Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

steve-the-edwards reviewed Jul 14, 2025

View reviewed changes

zach-klippenstein reviewed Jul 14, 2025

View reviewed changes

japplin reviewed Jul 14, 2025

View reviewed changes

Fix PR comments

eceef59

wenli-cai added 2 commits July 14, 2025 18:24

Fix unit tests

46765cb

Fix compose lint violations

dc8fae6

steve-the-edwards approved these changes Jul 18, 2025

View reviewed changes

steve-the-edwards reviewed Jul 18, 2025

View reviewed changes

Fix more PR comments

4fd32ef

steve-the-edwards approved these changes Jul 18, 2025

View reviewed changes

Merge branch 'main' into wenli/visualizer-ui-improvements

aed9dab

wenli-cai merged commit 314c0b3 into main Jul 18, 2025
64 checks passed

wenli-cai deleted the wenli/visualizer-ui-improvements branch July 18, 2025 17:53

	return copy(children = this.children.map { if (it.id == child.id) child else it })
	return copy(children = this.children.plus(child.id to child))

		val factor = 1f + (-scrollDelta * 0.1f)
		sandboxState.scale = (sandboxState.scale * factor).coerceIn(0.1f, 10f)

Workflow visualizer UI improvement #1366

Workflow visualizer UI improvement #1366

Uh oh!

Conversation

wenli-cai commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UI changes

Code changes

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenli-cai Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenli-cai commented Jul 10, 2025 •

edited

Loading

wenli-cai Jul 14, 2025 •

edited

Loading

wenli-cai Jul 14, 2025 •

edited

Loading

wenli-cai Jul 18, 2025 •

edited

Loading