Repository: cashapp/redwood
Pull request #479 (2022-10-28 02:36:01)

Throw an exception on duplicate widget ID
This happening is extremely implausible but technically possible and should only manifest in an insertion attempt for the root ID when the value wraps completely around. An insert for an existing non-zero ID should always be impossible, but is good to have covered.


Patch:
diff --git a/.github/workflows/todo.yaml b/.github/workflows/todo.yaml
index 68d6309e9..2337942b2 100644
--- a/.github/workflows/todo.yaml
+++ b/.github/workflows/todo.yaml
@@ -13,7 +13,7 @@ jobs:
       - uses: actions/checkout@v3
       # Note: Intentionally omitting the git submodule checkout
 
-      - uses: alstr/todo-to-issue-action@v4.8
+      - uses: alstr/todo-to-issue-action@v4.9
         id: todo
         with:
           TOKEN: ${{ secrets.GITHUB_TOKEN }}
diff --git a/redwood-protocol-widget/src/commonMain/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplay.kt b/redwood-protocol-widget/src/commonMain/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplay.kt
index 9bdfb82f1..2ef4286e8 100644
--- a/redwood-protocol-widget/src/commonMain/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplay.kt
+++ b/redwood-protocol-widget/src/commonMain/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplay.kt
@@ -43,7 +43,10 @@ public class ProtocolDisplay<T : Any>(
         is ChildrenDiff.Insert -> {
           val childWidget = factory.create(childrenDiff.kind) ?: continue
           children.insert(childrenDiff.index, childWidget)
-          nodes[childrenDiff.childId] = Node(childWidget, childrenDiff.id, children)
+          val old = nodes.put(childrenDiff.childId, Node(childWidget, childrenDiff.id, children))
+          require(old == null) {
+            "Insert attempted to replace existing widget with ID ${childrenDiff.childId.value}"
+          }
           node.childIds.add(childrenDiff.index, childrenDiff.childId)
         }
         is ChildrenDiff.Move -> {
diff --git a/redwood-treehouse/src/androidMain/kotlin/app/cash/redwood/treehouse/hotReloadAndroid.kt b/redwood-treehouse/src/androidMain/kotlin/app/cash/redwood/treehouse/hotReloadAndroid.kt
new file mode 100644
index 000000000..8e127d5a4
--- /dev/null
+++ b/redwood-treehouse/src/androidMain/kotlin/app/cash/redwood/treehouse/hotReloadAndroid.kt
@@ -0,0 +1,96 @@
+/*
+ * Copyright (C) 2022 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package app.cash.redwood.treehouse
+
+import java.util.concurrent.TimeUnit
+import kotlin.time.Duration.Companion.milliseconds
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.channels.awaitClose
+import kotlinx.coroutines.channels.trySendBlocking
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.flow.Flow
+import kotlinx.coroutines.flow.callbackFlow
+import kotlinx.coroutines.flow.emitAll
+import kotlinx.coroutines.flow.flatMapLatest
+import kotlinx.coroutines.flow.flow
+import kotlinx.coroutines.launch
+import okhttp3.HttpUrl.Companion.toHttpUrl
+import okhttp3.OkHttpClient
+import okhttp3.Request
+import okhttp3.Response
+import okhttp3.WebSocket
+import okhttp3.WebSocketListener
+
+@OptIn(ExperimentalCoroutinesApi::class)
+internal actual fun hotReloadFlow(flow: Flow<String>): Flow<String> {
+  return flow {
+    val wsClient: OkHttpClient = OkHttpClient.Builder()
+      .readTimeout(0, TimeUnit.MILLISECONDS)
+      .connectTimeout(1, TimeUnit.SECONDS)
+      .build()
+
+    emitAll(
+      flow.flatMapLatest { manifestUrl ->
+        var currentlyConnectedToWebSocket = false
+
+        val request: Request = Request.Builder()
+          .url(manifestUrl.toHttpUrl().resolve("/ws")!!)
+          .build()
+
+        return@flatMapLatest callbackFlow {
+          var ws: WebSocket? = null
+          launch {
+            while (true) {
+              if (!currentlyConnectedToWebSocket) {
+                trySendBlocking(manifestUrl)
+                println("we are here")
+                ws = wsClient.newWebSocket(
+                  request,
+                  object : WebSocketListener() {
+                    override fun onOpen(webSocket: WebSocket, response: Response) {
+                      currentlyConnectedToWebSocket = true
+                    }
+
+                    override fun onClosing(webSocket: WebSocket, code: Int, reason: String) {
+                      currentlyConnectedToWebSocket = false
+                      webSocket.close(1000, null)
+                    }
+
+                    override fun onFailure(webSocket: WebSocket, t: Throwable, response: Response?) {
+                      currentlyConnectedToWebSocket = false
+                      webSocket.close(1000, null)
+                    }
+
+                    override fun onMessage(webSocket: WebSocket, text: String) {
+                      if (text == "reload") {
+                        trySendBlocking(manifestUrl)
+                      }
+                    }
+                  },
+                )
+              }
+              delay(500.milliseconds)
+            }
+          }
+
+          awaitClose {
+            ws?.close(1000, null)
+          }
+        }
+      },
+    )
+  }
+}
diff --git a/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/TreehouseLauncher.kt b/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/TreehouseLauncher.kt
index 3180c2300..c614d47ab 100644
--- a/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/TreehouseLauncher.kt
+++ b/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/TreehouseLauncher.kt
@@ -20,7 +20,6 @@ import app.cash.zipline.loader.ManifestVerifier
 import app.cash.zipline.loader.ZiplineCache
 import app.cash.zipline.loader.ZiplineHttpClient
 import app.cash.zipline.loader.ZiplineLoader
-import kotlin.time.Duration.Companion.milliseconds
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.launch
@@ -106,7 +105,7 @@ public class TreehouseLauncher internal constructor(
     }
 
     val manifestUrlFlowForLoad = when (spec.freshCodePolicy) {
-      FreshCodePolicy.ALWAYS_REFRESH_IMMEDIATELY -> spec.manifestUrl.rebounce(500.milliseconds)
+      FreshCodePolicy.ALWAYS_REFRESH_IMMEDIATELY -> hotReloadFlow(spec.manifestUrl)
       else -> spec.manifestUrl
     }
 
diff --git a/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/hotReload.kt b/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/hotReload.kt
new file mode 100644
index 000000000..6de094202
--- /dev/null
+++ b/redwood-treehouse/src/hostMain/kotlin/app/cash/redwood/treehouse/hotReload.kt
@@ -0,0 +1,20 @@
+/*
+ * Copyright (C) 2022 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package app.cash.redwood.treehouse
+
+import kotlinx.coroutines.flow.Flow
+
+internal expect fun hotReloadFlow(flow: Flow<String>): Flow<String>
diff --git a/redwood-treehouse/src/nativeMain/kotlin/app/cash/redwood/treehouse/hotReloadNative.kt b/redwood-treehouse/src/nativeMain/kotlin/app/cash/redwood/treehouse/hotReloadNative.kt
new file mode 100644
index 000000000..094797310
--- /dev/null
+++ b/redwood-treehouse/src/nativeMain/kotlin/app/cash/redwood/treehouse/hotReloadNative.kt
@@ -0,0 +1,23 @@
+/*
+ * Copyright (C) 2022 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package app.cash.redwood.treehouse
+
+import kotlin.time.Duration.Companion.milliseconds
+import kotlinx.coroutines.flow.Flow
+
+internal actual fun hotReloadFlow(flow: Flow<String>): Flow<String> {
+  return flow.rebounce(500.milliseconds)
+}


Test patch:
diff --git a/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/DiffConsumingWidgetFactoryTest.kt b/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/DiffConsumingWidgetFactoryTest.kt
index 2b398250b..ee2df0d5a 100644
--- a/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/DiffConsumingWidgetFactoryTest.kt
+++ b/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/DiffConsumingWidgetFactoryTest.kt
@@ -16,19 +16,12 @@
 package app.cash.redwood.protocol.widget
 
 import app.cash.redwood.LayoutModifier
-import app.cash.redwood.layout.widget.RedwoodLayoutWidgetFactory
 import app.cash.redwood.protocol.Event
 import app.cash.redwood.protocol.EventSink
 import app.cash.redwood.protocol.Id
 import app.cash.redwood.protocol.PropertyDiff
 import example.redwood.compose.TestScope
-import example.redwood.widget.Button
 import example.redwood.widget.DiffConsumingExampleSchemaWidgetFactory
-import example.redwood.widget.ExampleSchemaWidgetFactory
-import example.redwood.widget.Row
-import example.redwood.widget.ScopedRow
-import example.redwood.widget.Space
-import example.redwood.widget.Text
 import example.redwood.widget.TextInput
 import kotlin.test.Test
 import kotlin.test.assertEquals
@@ -289,27 +282,6 @@ class DiffConsumingWidgetFactoryTest {
     assertEquals(Event(Id(1U), 4U, JsonPrimitive("PT10S")), eventSink.events.single())
   }
 
-  open class EmptyExampleSchemaWidgetFactory : ExampleSchemaWidgetFactory<Nothing> {
-    override val RedwoodLayout = object : RedwoodLayoutWidgetFactory<Nothing> {
-      override fun Column() = TODO("Not yet implemented")
-      override fun Row() = TODO("Not yet implemented")
-    }
-    override fun Row(): Row<Nothing> = TODO()
-    override fun ScopedRow(): ScopedRow<Nothing> = TODO()
-    override fun Text(): Text<Nothing> = TODO()
-    override fun Button() = object : Button<Nothing> {
-      override val value get() = TODO()
-      override var layoutModifiers: LayoutModifier
-        get() = TODO()
-        set(_) { TODO() }
-
-      override fun text(text: String?) = TODO()
-      override fun onClick(onClick: (() -> Unit)?) = TODO()
-    }
-    override fun TextInput(): TextInput<Nothing> = TODO()
-    override fun Space(): Space<Nothing> = TODO()
-  }
-
   class RecordingTextInput : TextInput<Nothing> {
     override val value get() = TODO()
     override var layoutModifiers: LayoutModifier = LayoutModifier
diff --git a/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/EmptyExampleSchemaWidgetFactory.kt b/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/EmptyExampleSchemaWidgetFactory.kt
new file mode 100644
index 000000000..ce3c5b387
--- /dev/null
+++ b/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/EmptyExampleSchemaWidgetFactory.kt
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2022 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package app.cash.redwood.protocol.widget
+
+import app.cash.redwood.LayoutModifier
+import app.cash.redwood.layout.widget.RedwoodLayoutWidgetFactory
+import example.redwood.widget.Button
+import example.redwood.widget.ExampleSchemaWidgetFactory
+import example.redwood.widget.Row
+import example.redwood.widget.ScopedRow
+import example.redwood.widget.Space
+import example.redwood.widget.Text
+import example.redwood.widget.TextInput
+
+open class EmptyExampleSchemaWidgetFactory : ExampleSchemaWidgetFactory<Nothing> {
+  override val RedwoodLayout = object : RedwoodLayoutWidgetFactory<Nothing> {
+    override fun Column() = TODO()
+    override fun Row() = TODO()
+  }
+  override fun Row(): Row<Nothing> = TODO()
+  override fun ScopedRow(): ScopedRow<Nothing> = TODO()
+  override fun Text(): Text<Nothing> = TODO()
+  override fun Button() = object : Button<Nothing> {
+    override val value get() = TODO()
+    override var layoutModifiers: LayoutModifier
+      get() = TODO()
+      set(_) { TODO() }
+
+    override fun text(text: String?) = TODO()
+    override fun onClick(onClick: (() -> Unit)?) = TODO()
+  }
+  override fun TextInput(): TextInput<Nothing> = TODO()
+  override fun Space(): Space<Nothing> = TODO()
+}
diff --git a/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplayTest.kt b/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplayTest.kt
new file mode 100644
index 000000000..2ba667b9c
--- /dev/null
+++ b/redwood-protocol-widget/src/commonTest/kotlin/app/cash/redwood/protocol/widget/ProtocolDisplayTest.kt
@@ -0,0 +1,75 @@
+/*
+ * Copyright (C) 2022 Square, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package app.cash.redwood.protocol.widget
+
+import app.cash.redwood.protocol.ChildrenDiff
+import app.cash.redwood.protocol.ChildrenDiff.Companion.RootChildrenTag
+import app.cash.redwood.protocol.Diff
+import app.cash.redwood.protocol.Id
+import app.cash.redwood.widget.MutableListChildren
+import example.redwood.widget.DiffConsumingExampleSchemaWidgetFactory
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
+
+class ProtocolDisplayTest {
+  @Test fun insertRootIdThrows() {
+    val protocolDisplay = ProtocolDisplay(
+      container = MutableListChildren(),
+      factory = DiffConsumingExampleSchemaWidgetFactory(EmptyExampleSchemaWidgetFactory()),
+      eventSink = ::error,
+    )
+    val diff = Diff(
+      childrenDiffs = listOf(
+        ChildrenDiff.Insert(
+          id = Id.Root,
+          tag = RootChildrenTag,
+          childId = Id.Root,
+          kind = 4 /* button */,
+          index = 0,
+        ),
+      ),
+    )
+    val t = assertFailsWith<IllegalArgumentException> {
+      protocolDisplay.sendDiff(diff)
+    }
+    assertEquals("Insert attempted to replace existing widget with ID 0", t.message)
+  }
+
+  @Test fun duplicateIdThrows() {
+    val protocolDisplay = ProtocolDisplay(
+      container = MutableListChildren(),
+      factory = DiffConsumingExampleSchemaWidgetFactory(EmptyExampleSchemaWidgetFactory()),
+      eventSink = ::error,
+    )
+    val diff = Diff(
+      childrenDiffs = listOf(
+        ChildrenDiff.Insert(
+          id = Id.Root,
+          tag = RootChildrenTag,
+          childId = Id(1U),
+          kind = 4 /* button */,
+          index = 0,
+        ),
+      ),
+    )
+    protocolDisplay.sendDiff(diff)
+    val t = assertFailsWith<IllegalArgumentException> {
+      protocolDisplay.sendDiff(diff)
+    }
+    assertEquals("Insert attempted to replace existing widget with ID 1", t.message)
+  }
+}