Merge pull request #1296 from square/sedwards/fix-no-change

Pass Rendering if any Conflations changed state

Author: steve-the-edwards

workflow-runtime/src/commonMain/kotlin/com/squareup/workflow1/RenderWorkflow.kt

@@ -165,9 +165,12 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
    * If [runtimeConfig] contains [RuntimeConfigOptions.RENDER_ONLY_WHEN_STATE_CHANGES] and
    * we have not changed state, then return true to short circuit the render loop.
    */
-  fun shouldShortCircuitForUnchangedState(actionResult: ActionProcessingResult): Boolean {
+  fun shouldShortCircuitForUnchangedState(
+    actionResult: ActionProcessingResult,
+    conflationHasChangedState: Boolean = false
+  ): Boolean {
     return runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES) &&
-      actionResult is ActionApplied<*> && !actionResult.stateChanged
+      actionResult is ActionApplied<*> && !actionResult.stateChanged && !conflationHasChangedState
   }
 
   scope.launch {
@@ -190,7 +193,9 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
       var nextRenderAndSnapshot: RenderingAndSnapshot<RenderingT> = runner.nextRendering()
 
       if (runtimeConfig.contains(CONFLATE_STALE_RENDERINGS)) {
+        var conflationHasChangedState = false
         conflate@ while (isActive && actionResult is ActionApplied<*> && actionResult.output == null) {
+          conflationHasChangedState = conflationHasChangedState || actionResult.stateChanged
           // We start by yielding, because if we are on an Unconfined dispatcher, we want to give
           // other signals (like Workers listening to the same result) a chance to get dispatched
           // and queue their actions.
@@ -202,7 +207,11 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
           if (actionResult == ActionsExhausted) break@conflate
 
           // Skip rendering if we had unchanged state, keep draining actions.
-          if (shouldShortCircuitForUnchangedState(actionResult)) {
+          if (shouldShortCircuitForUnchangedState(
+              actionResult = actionResult,
+              conflationHasChangedState = conflationHasChangedState
+            )
+          ) {
             sendOutput(actionResult, onOutput)
             continue@outer
           }

workflow-runtime/src/commonTest/kotlin/com/squareup/workflow1/RenderWorkflowInTest.kt

@@ -1393,7 +1393,6 @@ class RenderWorkflowInTest {
               trigger.asWorker()
             ) {
               action("") {
-                state = it
                 setOutput(it)
               }
             }
@@ -1403,10 +1402,9 @@ class RenderWorkflowInTest {
         val workflow = Workflow.stateful<String, String, String>(
           initialState = "unchanging state",
           render = { renderState ->
-            renderChild(childWorkflow) { childOutput ->
+            renderChild(childWorkflow) { _ ->
               action("childHandler") {
                 childHandlerActionExecuted++
-                state = childOutput
               }
             }
             runningWorker(
@@ -1632,6 +1630,90 @@ class RenderWorkflowInTest {
     }
   }
 
+  @Test
+  fun for_conflate_and_render_only_when_state_changed_we_do_not_conflate_stacked_actions_into_one_rendering_if_previous_rendering_changed() {
+    runtimeTestRunner.runParametrizedTest(
+      paramSource = runtimeOptions
+        .filter {
+          it.first.contains(CONFLATE_STALE_RENDERINGS) &&
+            it.first.contains(RENDER_ONLY_WHEN_STATE_CHANGES)
+        },
+      before = ::setup,
+    ) { (runtimeConfig: RuntimeConfig, workflowTracer: WorkflowTracer?, dispatcher: TestDispatcher) ->
+      runTest(dispatcher) {
+        check(runtimeConfig.contains(CONFLATE_STALE_RENDERINGS))
+        check(runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES))
+
+        var childHandlerActionExecuted = false
+        val trigger = MutableSharedFlow<String>()
+        val emitted = mutableListOf<String>()
+
+        val childWorkflow = Workflow.stateful<String, String, String>(
+          initialState = "unchanging state",
+          render = { renderState ->
+            runningWorker(
+              trigger.asWorker()
+            ) {
+              action("") {
+                state = it
+                setOutput(it)
+              }
+            }
+            renderState
+          }
+        )
+        val workflow = Workflow.stateful<String, String, String>(
+          initialState = "unchanging state",
+          render = { renderState ->
+            renderChild(childWorkflow) { childOutput ->
+              action("childHandler") {
+                childHandlerActionExecuted = true
+                state = "$childOutput+update"
+              }
+            }
+            runningWorker(
+              trigger.asWorker()
+            ) {
+              action("") {
+                // no state change now!
+              }
+            }
+            renderState
+          }
+        )
+        val props = MutableStateFlow(Unit)
+        val renderings = renderWorkflowIn(
+          workflow = workflow,
+          scope = backgroundScope,
+          props = props,
+          runtimeConfig = runtimeConfig,
+          workflowTracer = workflowTracer,
+        ) { }
+
+        val collectionJob = launch {
+          // Collect this unconfined so we can get all the renderings faster than actions can
+          // be processed.
+          renderings.collect {
+            emitted += it.rendering
+          }
+        }
+        advanceIfStandard(dispatcher)
+
+        launch {
+          trigger.emit("changed state")
+        }
+        advanceIfStandard(dispatcher)
+
+        collectionJob.cancel()
+
+        // 2 renderings.
+        assertEquals(2, emitted.size)
+        assertEquals("changed state+update", emitted.last())
+        assertTrue(childHandlerActionExecuted)
+      }
+    }
+  }
+
   private class ExpectedException : RuntimeException()
 
   private fun <T1, T2> cartesianProduct(