Merge pull request #1306 from square/sedward/csr-handling

CSR pass old rendering, do not render again if can skip

Author: steve-the-edwards

workflow-runtime/api/workflow-runtime.api

@@ -88,7 +88,10 @@ public final class com/squareup/workflow1/WorkflowInterceptor$RenderContextInter
 }
 
 public final class com/squareup/workflow1/WorkflowInterceptor$RenderPassSkipped : com/squareup/workflow1/WorkflowInterceptor$RuntimeLoopOutcome {
-	public static final field INSTANCE Lcom/squareup/workflow1/WorkflowInterceptor$RenderPassSkipped;
+	public fun <init> ()V
+	public fun <init> (Z)V
+	public synthetic fun <init> (ZILkotlin/jvm/internal/DefaultConstructorMarker;)V
+	public final fun getEndOfTick ()Z
 }
 
 public final class com/squareup/workflow1/WorkflowInterceptor$RenderPassesComplete : com/squareup/workflow1/WorkflowInterceptor$RuntimeLoopOutcome {

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

@@ -170,12 +170,10 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
    */
   fun shouldShortCircuitForUnchangedState(
     actionResult: ActionProcessingResult,
-    conflationHasChangedState: Boolean = false
   ): Boolean {
     return runtimeConfig.contains(RENDER_ONLY_WHEN_STATE_CHANGES) &&
       actionResult is ActionApplied<*> &&
-      !actionResult.stateChanged &&
-      !conflationHasChangedState
+      !actionResult.stateChanged
   }
 
   scope.launch {
@@ -186,7 +184,7 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
       var actionResult: ActionProcessingResult = runner.processAction()
 
       if (shouldShortCircuitForUnchangedState(actionResult)) {
-        chainedInterceptor.onRuntimeLoopTick(RenderPassSkipped)
+        chainedInterceptor.onRuntimeLoopTick(RenderPassSkipped())
         sendOutput(actionResult, onOutput)
         continue@outer
       }
@@ -211,10 +209,15 @@ public fun <PropsT, OutputT, RenderingT> renderWorkflowIn(
           // Skip rendering if we had unchanged state, keep draining actions.
           if (shouldShortCircuitForUnchangedState(
               actionResult = actionResult,
-              conflationHasChangedState = conflationHasChangedState
             )
           ) {
-            chainedInterceptor.onRuntimeLoopTick(RenderPassSkipped)
+            if (conflationHasChangedState) {
+              chainedInterceptor.onRuntimeLoopTick(RenderPassSkipped(endOfTick = false))
+              // An earlier render changed state, so we need to pass that to the UI then we
+              // can skip this render.
+              break@conflate
+            }
+            chainedInterceptor.onRuntimeLoopTick(RenderPassSkipped())
             sendOutput(actionResult, onOutput)
             continue@outer
           }

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

@@ -157,7 +157,23 @@ public interface WorkflowInterceptor {
   ): Unit = Unit
 
   public sealed interface RuntimeLoopOutcome
-  public object RenderPassSkipped : RuntimeLoopOutcome
+
+  /**
+   * @param endOfTick This is true if this skip was the end of the loop iteration (i.e. no update
+   *        was passed to the UI. It is false otherwise. An example of when it can be false is if
+   *        we have an action that causes a state change, but then we can process more while the
+   *        `CONFLATE_STALE_RENDERINGS` optimization is enabled. We may skip rendering based on
+   *        the subsequent action not changing state, but we will need to finish the loop and update
+   *        the UI from the previous action that changed state.
+   */
+  public class RenderPassSkipped(
+    public val endOfTick: Boolean = true
+  ) : RuntimeLoopOutcome
+
+  /**
+   * @param renderingAndSnapshot This is the rendering and snapshot that was passed out of the
+   *        Workflow runtime.
+   */
   public class RenderPassesComplete<R>(
     public val renderingAndSnapshot: RenderingAndSnapshot<R>
   ) : RuntimeLoopOutcome

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

@@ -1887,6 +1887,95 @@ class RenderWorkflowInTest {
     }
   }
 
+  @Test
+  fun for_conflate_and_render_only_when_state_changed_we_do_not_render_again_if_only_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>()
+        var renderCount = 0
+
+        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.also {
+              renderCount++
+            }
+          }
+        )
+        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())
+        // Only 2 times rendered, the initial + the update (not 3).
+        assertEquals(2, renderCount)
+        assertTrue(childHandlerActionExecuted)
+      }
+    }
+  }
+
   private class ExpectedException : RuntimeException()
 
   private fun <T1, T2> cartesianProduct(