Slightly improve initial sync hangs (#1140)

CHANGELOG.md

@@ -33,11 +33,13 @@ Making a new release? Simply add the new header with the version and date undern
 
 * Fixed a bug where the last sync timestamp was not updating correctly in the plugin ([#1132])
 * Improved the reliability of sync replacements by adding better error handling and recovery ([#1135])
+* Small improvements to stability when syncing massive projects ([#1140])
 * Added support for JSON comments and trailing commas in project, meta, and model json files ([#1144])
 * Added `sourcemap.json` into the default `.gitignore` files ([#1145])
 
 [#1132]: https://github.com/rojo-rbx/rojo/pull/1132
 [#1135]: https://github.com/rojo-rbx/rojo/pull/1135
+[#1140]: https://github.com/rojo-rbx/rojo/pull/1140
 [#1144]: https://github.com/rojo-rbx/rojo/pull/1144
 [#1145]: https://github.com/rojo-rbx/rojo/pull/1145
 

plugin/src/App/StatusPages/Confirming.lua

@@ -4,8 +4,6 @@ local Packages = Rojo.Packages
 
 local Roact = require(Packages.Roact)
 
-local Timer = require(Plugin.Timer)
-local PatchTree = require(Plugin.PatchTree)
 local Settings = require(Plugin.Settings)
 local Theme = require(Plugin.App.Theme)
 local TextButton = require(Plugin.App.Components.TextButton)
@@ -24,7 +22,6 @@ function ConfirmingPage:init()
 	self.containerSize, self.setContainerSize = Roact.createBinding(Vector2.new(0, 0))
 
 	self:setState({
-		patchTree = nil,
 		showingStringDiff = false,
 		oldString = "",
 		newString = "",
@@ -32,28 +29,6 @@ function ConfirmingPage:init()
 		oldTable = {},
 		newTable = {},
 	})
-
-	if self.props.confirmData and self.props.confirmData.patch and self.props.confirmData.instanceMap then
-		self:buildPatchTree()
-	end
-end
-
-function ConfirmingPage:didUpdate(prevProps)
-	if prevProps.confirmData ~= self.props.confirmData then
-		self:buildPatchTree()
-	end
-end
-
-function ConfirmingPage:buildPatchTree()
-	Timer.start("ConfirmingPage:buildPatchTree")
-	self:setState({
-		patchTree = PatchTree.build(
-			self.props.confirmData.patch,
-			self.props.confirmData.instanceMap,
-			{ "Property", "Current", "Incoming" }
-		),
-	})
-	Timer.stop()
 end
 
 function ConfirmingPage:render()
@@ -79,7 +54,7 @@ function ConfirmingPage:render()
 				transparency = self.props.transparency,
 				layoutOrder = 3,
 
-				patchTree = self.state.patchTree,
+				patchTree = self.props.patchTree,
 
 				showStringDiff = function(oldString: string, newString: string)
 					self:setState({

plugin/src/App/StatusPages/Connecting.lua

@@ -4,6 +4,8 @@ local Packages = Rojo.Packages
 
 local Roact = require(Packages.Roact)
 
+local Theme = require(Plugin.App.Theme)
+
 local Spinner = require(Plugin.App.Components.Spinner)
 
 local e = Roact.createElement
@@ -11,11 +13,35 @@ local e = Roact.createElement
 local ConnectingPage = Roact.Component:extend("ConnectingPage")
 
 function ConnectingPage:render()
-	return e(Spinner, {
-		position = UDim2.new(0.5, 0, 0.5, 0),
-		anchorPoint = Vector2.new(0.5, 0.5),
-		transparency = self.props.transparency,
-	})
+	return Theme.with(function(theme)
+		return e("Frame", {
+			Size = UDim2.new(1, 0, 1, 0),
+			BackgroundTransparency = 1,
+		}, {
+			Spinner = e(Spinner, {
+				position = UDim2.new(0.5, 0, 0.5, 0),
+				anchorPoint = Vector2.new(0.5, 0.5),
+				transparency = self.props.transparency,
+			}),
+			Text = if type(self.props.text) == "string" and #self.props.text > 0
+				then e("TextLabel", {
+					Text = self.props.text,
+					Position = UDim2.new(0.5, 0, 0.5, 30),
+					Size = UDim2.new(1, -40, 0.5, -40),
+					AnchorPoint = Vector2.new(0.5, 0),
+					TextXAlignment = Enum.TextXAlignment.Center,
+					TextYAlignment = Enum.TextYAlignment.Top,
+					RichText = true,
+					FontFace = theme.Font.Thin,
+					TextSize = theme.TextSize.Medium,
+					TextColor3 = theme.SubTextColor,
+					TextTruncate = Enum.TextTruncate.AtEnd,
+					TextTransparency = self.props.transparency,
+					BackgroundTransparency = 1,
+				})
+				else nil,
+		})
+	end)
 end
 
 return ConnectingPage

plugin/src/App/init.lua

@@ -595,6 +595,12 @@ function App:startSession()
 		twoWaySync = Settings:get("twoWaySync"),
 	})
 
+	serveSession:setUpdateLoadingTextCallback(function(text: string)
+		self:setState({
+			connectingText = text,
+		})
+	end)
+
 	self.cleanupPrecommit = serveSession:hookPrecommit(function(patch, instanceMap)
 		-- Build new tree for patch
 		self:setState({
@@ -759,11 +765,13 @@ function App:startSession()
 			end
 		end
 
+		self:setState({
+			connectingText = "Computing diff view...",
+		})
 		self:setState({
 			appStatus = AppStatus.Confirming,
+			patchTree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }),
 			confirmData = {
-				instanceMap = instanceMap,
-				patch = patch,
 				serverInfo = serverInfo,
 			},
 			toolbarIcon = Assets.Images.PluginButton,
@@ -874,6 +882,7 @@ function App:render()
 
 					ConfirmingPage = createPageElement(AppStatus.Confirming, {
 						confirmData = self.state.confirmData,
+						patchTree = self.state.patchTree,
 						createPopup = not self.state.guiEnabled,
 
 						onAbort = function()
@@ -887,7 +896,9 @@ function App:render()
 						end,
 					}),
 
-					Connecting = createPageElement(AppStatus.Connecting),
+					Connecting = createPageElement(AppStatus.Connecting, {
+						text = self.state.connectingText,
+					}),
 
 					Connected = createPageElement(AppStatus.Connected, {
 						projectName = self.state.projectName,

plugin/src/PatchTree.lua

@@ -16,6 +16,14 @@ local Types = require(Plugin.Types)
 local decodeValue = require(Plugin.Reconciler.decodeValue)
 local getProperty = require(Plugin.Reconciler.getProperty)
 
+local function yieldIfNeeded(clock)
+	if os.clock() - clock > 1 / 20 then
+		task.wait()
+		return os.clock()
+	end
+	return clock
+end
+
 local function alphabeticalNext(t, state)
 	-- Equivalent of the next function, but returns the keys in the alphabetic
 	-- order of node names. We use a temporary ordered key table that is stored in the
@@ -132,7 +140,6 @@ end
 -- props must contain id, and cannot contain children or parentId
 -- other than those three, it can hold anything
 function Tree:addNode(parent, props)
-	Timer.start("Tree:addNode")
 	assert(props.id, "props must contain id")
 
 	parent = parent or "ROOT"
@@ -143,7 +150,6 @@ function Tree:addNode(parent, props)
 		for k, v in props do
 			node[k] = v
 		end
-		Timer.stop()
 		return node
 	end
 
@@ -154,25 +160,25 @@ function Tree:addNode(parent, props)
 	local parentNode = self:getNode(parent)
 	if not parentNode then
 		Log.warn("Failed to create node since parent doesnt exist: {}, {}", parent, props)
-		Timer.stop()
 		return
 	end
 
 	parentNode.children[node.id] = node
 	self.idToNode[node.id] = node
 
-	Timer.stop()
 	return node
 end
 
 -- Given a list of ancestor ids in descending order, builds the nodes for them
 -- using the patch and instanceMap info
 function Tree:buildAncestryNodes(previousId: string?, ancestryIds: { string }, patch, instanceMap)
-	Timer.start("Tree:buildAncestryNodes")
+	local clock = os.clock()
 	-- Build nodes for ancestry by going up the tree
 	previousId = previousId or "ROOT"
 
 	for _, ancestorId in ancestryIds do
+		clock = yieldIfNeeded(clock)
+
 		local value = instanceMap.fromIds[ancestorId] or patch.added[ancestorId]
 		if not value then
 			Log.warn("Failed to find ancestor object for " .. ancestorId)
@@ -186,8 +192,6 @@ function Tree:buildAncestryNodes(previousId: string?, ancestryIds: { string }, p
 		})
 		previousId = ancestorId
 	end
-
-	Timer.stop()
 end
 
 local PatchTree = {}
@@ -196,12 +200,16 @@ local PatchTree = {}
 -- uses changeListHeaders in node.changeList
 function PatchTree.build(patch, instanceMap, changeListHeaders)
 	Timer.start("PatchTree.build")
+	local clock = os.clock()
+
 	local tree = Tree.new()
 
 	local knownAncestors = {}
 
 	Timer.start("patch.updated")
 	for _, change in patch.updated do
+		clock = yieldIfNeeded(clock)
+
 		local instance = instanceMap.fromIds[change.id]
 		if not instance then
 			continue
@@ -281,6 +289,8 @@ function PatchTree.build(patch, instanceMap, changeListHeaders)
 
 	Timer.start("patch.removed")
 	for _, idOrInstance in patch.removed do
+		clock = yieldIfNeeded(clock)
+
 		local instance = if Types.RbxId(idOrInstance) then instanceMap.fromIds[idOrInstance] else idOrInstance
 		if not instance then
 			-- If we're viewing a past patch, the instance is already removed
@@ -325,6 +335,8 @@ function PatchTree.build(patch, instanceMap, changeListHeaders)
 
 	Timer.start("patch.added")
 	for id, change in patch.added do
+		clock = yieldIfNeeded(clock)
+
 		-- Gather ancestors from existing DOM or future additions
 		local ancestryIds = {}
 		local parentId = change.Parent

plugin/src/ServeSession.lua

@@ -107,6 +107,7 @@ function ServeSession.new(options)
 		__connections = connections,
 		__precommitCallbacks = {},
 		__postcommitCallbacks = {},
+		__updateLoadingText = function() end,
 	}
 
 	setmetatable(self, ServeSession)
@@ -137,6 +138,14 @@ function ServeSession:setConfirmCallback(callback)
 	self.__userConfirmCallback = callback
 end
 
+function ServeSession:setUpdateLoadingTextCallback(callback)
+	self.__updateLoadingText = callback
+end
+
+function ServeSession:setLoadingText(text: string)
+	self.__updateLoadingText(text)
+end
+
 --[=[
 	Hooks a function to run before patch application.
 	The provided function is called with the incoming patch and an InstanceMap
@@ -181,11 +190,14 @@ end
 
 function ServeSession:start()
 	self:__setStatus(Status.Connecting)
+	self:setLoadingText("Connecting to server...")
 
 	self.__apiContext
 		:connect()
 		:andThen(function(serverInfo)
+			self:setLoadingText("Loading initial data from server...")
 			return self:__initialSync(serverInfo):andThen(function()
+				self:setLoadingText("Starting sync loop...")
 				self:__setStatus(Status.Connected, serverInfo.projectName)
 				self:__applyGameAndPlaceId(serverInfo)
 
@@ -449,11 +461,13 @@ function ServeSession:__initialSync(serverInfo)
 		-- For any instances that line up with the Rojo server's view, start
 		-- tracking them in the reconciler.
 		Log.trace("Matching existing Roblox instances to Rojo IDs")
+		self:setLoadingText("Hydrating instance map...")
 		self.__reconciler:hydrate(readResponseBody.instances, serverInfo.rootInstanceId, game)
 
 		-- Calculate the initial patch to apply to the DataModel to catch us
 		-- up to what Rojo thinks the place should look like.
 		Log.trace("Computing changes that plugin needs to make to catch up to server...")
+		self:setLoadingText("Finding differences between server and Studio...")
 		local success, catchUpPatch =
 			self.__reconciler:diff(readResponseBody.instances, serverInfo.rootInstanceId, game)