Fix the diff visualizer of connected sessions (#674)

Clicking on the "X changes X ago" message opens up a handy diff
visualizer to see what those changes were. However, it had quite a few
issues that needed fixing.

- Disconnecting a session with it expanded caused an error as it tried
to read the serveSession that no longer exists during the page fade
transition. (#671)
- Resolved by converting to stateful component and holding the
serveSession during the lifetime to ensure it can render the last known
changes during the fade transition
- Leaving it open while new changes are synced did not update the
visualizer
- The patch data was piggybacking on an existing binding, which meant
that new patches did not trigger rerender.
    - Resolved by converting to state
    - Also made some improvements to that old binding
- Moved from app to connected page for better organization and
separation of duties
      - No more useless updates causing rerenders with no real change
- Scroll window child component wouldn't actually display the updated
visuals
    - Resolved by making major improvements to VirtualScroller
      - Made more robust against edge case states
      - Made smarter about knowing when it needs to refresh

As you can see in this slow motion GIF, it works now.

![slowedDemo](https://github.com/rojo-rbx/rojo/assets/40185666/c9fc8489-72a9-47be-ae45-9c420e1535d4)

plugin/src/App/Components/PatchVisualizer/DomLabel.lua

@@ -40,11 +40,6 @@ end
 local DomLabel = Roact.Component:extend("DomLabel")
 
 function DomLabel:init()
-	self.maxElementHeight = 0
-	if self.props.changeList then
-		self.maxElementHeight = math.clamp(#self.props.changeList * 30, 30, 30 * 6)
-	end
-
 	local initHeight = self.props.elementHeight:getValue()
 	self.expanded = initHeight > 30
 
@@ -118,7 +113,8 @@ function DomLabel:render()
 					Size = UDim2.new(1, 0, 1, 0),
 					[Roact.Event.Activated] = function()
 						self.expanded = not self.expanded
-						self.motor:setGoal(Flipper.Spring.new((self.expanded and self.maxElementHeight or 0) + 30, {
+						local goalHeight = 30 + (if self.expanded then math.clamp(#self.props.changeList * 30, 30, 30 * 6) else 0)
+						self.motor:setGoal(Flipper.Spring.new(goalHeight, {
 							frequency = 5,
 							dampingRatio = 1,
 						}))

plugin/src/App/Components/VirtualScroller.lua

@@ -99,18 +99,30 @@ function VirtualScroller:refresh()
 	})
 end
 
+function VirtualScroller:didUpdate(previousProps)
+	if self.props.count ~= previousProps.count then
+		-- Items have changed, so we need to refresh
+		self:refresh()
+	end
+end
+
 function VirtualScroller:render()
 	local props, state = self.props, self.state
 
 	local items = {}
 	for i = state.Start, state.End do
+		local content = props.render(i)
+		if content == nil then
+			continue
+		end
+
 		items["Item" .. i] = e("Frame", {
 			LayoutOrder = i,
 			Size = props.getHeightBinding(i):map(function(height)
 				return UDim2.new(1, 0, 0, height)
 			end),
 			BackgroundTransparency = 1,
-		}, props.render(i))
+		}, content)
 	end
 
 	return Theme.with(function(theme)

plugin/src/App/StatusPages/Connected.lua

@@ -26,57 +26,65 @@ function timeSinceText(elapsed: number): string
 
 	local ageText = string.format("%d seconds ago", elapsed)
 
-	for _,UnitData in ipairs(AGE_UNITS) do
+	for _, UnitData in ipairs(AGE_UNITS) do
 		local UnitSeconds, UnitName = UnitData[1], UnitData[2]
 		if elapsed > UnitSeconds then
-			local c = math.floor(elapsed/UnitSeconds)
-			ageText = string.format("%d %s%s ago", c, UnitName, c>1 and "s" or "")
+			local c = math.floor(elapsed / UnitSeconds)
+			ageText = string.format("%d %s%s ago", c, UnitName, c > 1 and "s" or "")
 			break
 		end
 	end
 
 	return ageText
 end
 
-local function ChangesDrawer(props)
-	if props.rendered == false then
+local ChangesDrawer = Roact.Component:extend("ConnectedPage")
+
+function ChangesDrawer:init()
+	-- Hold onto the serve session during the lifecycle of this component
+	-- so that it can still render during the fade out after disconnecting
+	self.serveSession = self.props.serveSession
+end
+
+function ChangesDrawer:render()
+	if self.props.rendered == false or self.serveSession == nil then
 		return nil
 	end
 
 	return Theme.with(function(theme)
 		return e(BorderedContainer, {
-			transparency = props.transparency,
-			size = props.height:map(function(y)
+			transparency = self.props.transparency,
+			size = self.props.height:map(function(y)
 				return UDim2.new(1, 0, y, -180 * y)
 			end),
 			position = UDim2.new(0, 0, 1, 0),
 			anchorPoint = Vector2.new(0, 1),
-			layoutOrder = props.layoutOrder,
+			layoutOrder = self.props.layoutOrder,
 		}, {
 			Close = e(IconButton, {
 				icon = Assets.Images.Icons.Close,
 				iconSize = 24,
 				color = theme.ConnectionDetails.DisconnectColor,
-				transparency = props.transparency,
+				transparency = self.props.transparency,
 
 				position = UDim2.new(1, 0, 0, 0),
 				anchorPoint = Vector2.new(1, 0),
 
-				onClick = props.onClose,
+				onClick = self.props.onClose,
 			}, {
 				Tip = e(Tooltip.Trigger, {
-					text = "Close the patch visualizer"
+					text = "Close the patch visualizer",
 				}),
 			}),
 
 			PatchVisualizer = e(PatchVisualizer, {
 				size = UDim2.new(1, 0, 1, 0),
-				transparency = props.transparency,
+				transparency = self.props.transparency,
 				layoutOrder = 3,
 
-				columnVisibility = {true, false, true},
-				patch = props.patchInfo:getValue().patch,
-				instanceMap = props.serveSession.__instanceMap,
+				columnVisibility = { true, false, true },
+				patch = self.props.patch,
+				instanceMap = self.serveSession.__instanceMap,
 			}),
 		})
 	end)
@@ -91,7 +99,7 @@ local function ConnectionDetails(props)
 		}, {
 			TextContainer = e("Frame", {
 				Size = UDim2.new(1, 0, 1, 0),
-				BackgroundTransparency = 1
+				BackgroundTransparency = 1,
 			}, {
 				ProjectName = e("TextLabel", {
 					Text = props.projectName,
@@ -141,7 +149,7 @@ local function ConnectionDetails(props)
 				onClick = props.onDisconnect,
 			}, {
 				Tip = e(Tooltip.Trigger, {
-					text = "Disconnect from the Rojo sync server"
+					text = "Disconnect from the Rojo sync server",
 				}),
 			}),
 
@@ -155,6 +163,18 @@ end
 
 local ConnectedPage = Roact.Component:extend("ConnectedPage")
 
+function ConnectedPage:getChangeInfoText()
+	local patchData = self.props.patchData
+	if patchData == nil then
+		return ""
+	end
+
+	local elapsed = os.time() - patchData.timestamp
+	local changes = PatchSet.countChanges(patchData.patch)
+
+	return string.format("<i>Synced %d change%s %s</i>", changes, changes == 1 and "" or "s", timeSinceText(elapsed))
+end
+
 function ConnectedPage:init()
 	self.changeDrawerMotor = Flipper.SingleMotor.new(0)
 	self.changeDrawerHeight = bindingUtil.fromMotor(self.changeDrawerMotor)
@@ -176,6 +196,34 @@ function ConnectedPage:init()
 	self:setState({
 		renderChanges = false,
 	})
+
+	self.changeInfoText, self.setChangeInfoText = Roact.createBinding("")
+
+	self.changeInfoTextUpdater = task.defer(function()
+		while true do
+			self.setChangeInfoText(self:getChangeInfoText())
+
+			local elapsed = os.time() - self.props.patchData.timestamp
+			local updateInterval = 1
+
+			-- Update timestamp text as frequently as currently needed
+			for _, UnitData in ipairs(AGE_UNITS) do
+				local UnitSeconds = UnitData[1]
+				if elapsed >= UnitSeconds then
+					updateInterval = UnitSeconds
+					break
+				end
+			end
+
+			task.wait(updateInterval)
+		end
+	end)
+end
+
+function ConnectedPage:willUnmount()
+	if self.changeInfoTextUpdater then
+		task.cancel(self.changeInfoTextUpdater)
+	end
 end
 
 function ConnectedPage:render()
@@ -208,15 +256,7 @@ function ConnectedPage:render()
 			}),
 
 			ChangeInfo = e("TextButton", {
-				Text = self.props.patchInfo:map(function(info)
-					local changes = PatchSet.countChanges(info.patch)
-					return string.format(
-						"<i>Synced %d change%s %s</i>",
-						changes,
-						changes == 1 and "" or "s",
-						timeSinceText(os.time() - info.timestamp)
-					)
-				end),
+				Text = self.changeInfoText,
 				Font = Enum.Font.Gotham,
 				TextSize = 14,
 				TextWrapped = true,
@@ -249,7 +289,7 @@ function ConnectedPage:render()
 			ChangesDrawer = e(ChangesDrawer, {
 				rendered = self.state.renderChanges,
 				transparency = self.props.transparency,
-				patchInfo = self.props.patchInfo,
+				patch = self.props.patchData.patch,
 				serveSession = self.props.serveSession,
 				height = self.changeDrawerHeight,
 				layoutOrder = 4,

plugin/src/App/init.lua

@@ -51,17 +51,17 @@ function App:init()
 	self.host, self.setHost = Roact.createBinding(priorHost or "")
 	self.port, self.setPort = Roact.createBinding(priorPort or "")
 
-	self.patchInfo, self.setPatchInfo = Roact.createBinding({
-		patch = PatchSet.newEmpty(),
-		timestamp = os.time(),
-	})
 	self.confirmationBindable = Instance.new("BindableEvent")
 	self.confirmationEvent = self.confirmationBindable.Event
 
 	self:setState({
 		appStatus = AppStatus.NotConnected,
 		guiEnabled = false,
 		confirmData = {},
+		patchData = {
+			patch = PatchSet.newEmpty(),
+			timestamp = os.time(),
+		},
 		notifications = {},
 		toolbarIcon = Assets.Images.PluginButton,
 	})
@@ -227,21 +227,25 @@ function App:startSession()
 
 		local now = os.time()
 
-		local old = self.patchInfo:getValue()
+		local old = self.state.patchData
 		if now - old.timestamp < 2 then
 			-- Patches that apply in the same second are
 			-- considered to be part of the same change for human clarity
 			local merged = PatchSet.newEmpty()
 			PatchSet.assign(merged, old.patch, patch)
 
-			self.setPatchInfo({
-				patch = merged,
-				timestamp = now,
+			self:setState({
+				patchData = {
+					patch = merged,
+					timestamp = now,
+				},
 			})
 		else
-			self.setPatchInfo({
-				patch = patch,
-				timestamp = now,
+			self:setState({
+				patchData = {
+					patch = patch,
+					timestamp = now,
+				},
 			})
 		end
 	end)
@@ -318,16 +322,6 @@ function App:startSession()
 	serveSession:start()
 
 	self.serveSession = serveSession
-
-	task.defer(function()
-		while self.serveSession == serveSession do
-			-- Trigger rerender to update timestamp text
-			local patchInfo = table.clone(self.patchInfo:getValue())
-			self.setPatchInfo(patchInfo)
-			local elapsed = os.time() - patchInfo.timestamp
-			task.wait(elapsed < 60 and 1 or elapsed / 5)
-		end
-	end)
 end
 
 function App:endSession()
@@ -429,7 +423,7 @@ function App:render()
 					Connected = createPageElement(AppStatus.Connected, {
 						projectName = self.state.projectName,
 						address = self.state.address,
-						patchInfo = self.patchInfo,
+						patchData = self.state.patchData,
 						serveSession = self.serveSession,
 
 						onDisconnect = function()