Backport #848, #846, #845, #844 to 7.4 (#849)

Co-authored-by: boatbomber <zack@boatbomber.com>

.github/workflows/ci.yml

@@ -72,7 +72,7 @@ jobs:
       run: cargo build --locked --verbose
 
   lint:
-    name: Rustfmt, Clippy, & Stylua
+    name: Rustfmt, Clippy, Stylua, & Selene
     runs-on: ubuntu-latest
 
     steps:
@@ -98,6 +98,9 @@ jobs:
     - name: Stylua
       run: stylua --check plugin/src
 
+    - name: Selene
+      run: selene plugin/src
+
     - name: Rustfmt
       run: cargo fmt -- --check
 

CHANGELOG.md

@@ -1,6 +1,12 @@
 # Rojo Changelog
 
 ## Unreleased Changes
+* Fixed Rojo plugin failing to connect when project contains certain unreadable properties ([#848])
+* Fixed various cases where patch visualizer would not display sync failures ([#845], [#844])
+
+[#848]: https://github.com/rojo-rbx/rojo/pull/848
+[#845]: https://github.com/rojo-rbx/rojo/pull/845
+[#844]: https://github.com/rojo-rbx/rojo/pull/844
 
 ## [7.4.0] - January 16, 2024
 * Improved the visualization for array properties like Tags ([#829])

aftman.toml

@@ -1,5 +1,5 @@
 [tools]
 rojo = "rojo-rbx/rojo@7.3.0"
-selene = "Kampfkarren/selene@0.25.0"
+selene = "Kampfkarren/selene@0.26.1"
 stylua = "JohnnyMorganz/stylua@0.18.2"
 run-in-roblox = "rojo-rbx/run-in-roblox@0.3.0"

plugin/src/ApiContext.lua

@@ -185,10 +185,10 @@ function ApiContext:write(patch)
 
 	body = Http.jsonEncode(body)
 
-	return Http.post(url, body):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(body)
-		Log.info("Write response: {:?}", body)
+	return Http.post(url, body):andThen(rejectFailedRequests):andThen(Http.Response.json):andThen(function(responseBody)
+		Log.info("Write response: {:?}", responseBody)
 
-		return body
+		return responseBody
 	end)
 end
 

plugin/src/App/Components/Dropdown.lua

@@ -167,7 +167,7 @@ function Dropdown:render()
 								self.setContentSize(object.AbsoluteContentSize)
 							end,
 						}),
-						Roact.createFragment(optionButtons),
+						Options = Roact.createFragment(optionButtons),
 					}),
 				})
 				else nil,

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

@@ -57,7 +57,7 @@ local function DisplayValue(props)
 				-- We don't need to support mixed tables, so checking the first key is enough
 				-- to determine if it's a simple array
 				local out, i = table.create(#props.value), 0
-				for k, v in props.value do
+				for _, v in props.value do
 					i += 1
 
 					-- Wrap strings in quotes

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

@@ -97,21 +97,16 @@ function DomLabel:render()
 		-- Line guides help indent depth remain readable
 		local lineGuides = {}
 		for i = 1, props.depth or 0 do
-			table.insert(
-				lineGuides,
-				e("Frame", {
-					Name = "Line_" .. i,
-					Size = UDim2.new(0, 2, 1, 2),
-					Position = UDim2.new(0, (20 * i) + 15, 0, -1),
-					BorderSizePixel = 0,
-					BackgroundTransparency = props.transparency,
-					BackgroundColor3 = theme.BorderedContainer.BorderColor,
-				})
-			)
+			lineGuides["Line_" .. i] = e("Frame", {
+				Size = UDim2.new(0, 2, 1, 2),
+				Position = UDim2.new(0, (20 * i) + 15, 0, -1),
+				BorderSizePixel = 0,
+				BackgroundTransparency = props.transparency,
+				BackgroundColor3 = theme.BorderedContainer.BorderColor,
+			})
 		end
 
 		return e("Frame", {
-			Name = "Change",
 			ClipsDescendants = true,
 			BackgroundColor3 = if props.patchType then theme.Diff[props.patchType] else nil,
 			BorderSizePixel = 0,

plugin/src/App/Components/TextButton.lua

@@ -42,7 +42,7 @@ end
 function TextButton:render()
 	return Theme.with(function(theme)
 		local textSize =
-			TextService:GetTextSize(self.props.text, 18, Enum.Font.GothamSemibold, Vector2.new(math.huge, math.huge))
+			TextService:GetTextSize(self.props.text, 18, Enum.Font.GothamMedium, Vector2.new(math.huge, math.huge))
 
 		local style = self.props.style
 
@@ -83,7 +83,7 @@ function TextButton:render()
 
 			Text = e("TextLabel", {
 				Text = self.props.text,
-				Font = Enum.Font.GothamSemibold,
+				Font = Enum.Font.GothamMedium,
 				TextSize = 18,
 				TextColor3 = bindingUtil.mapLerp(bindingEnabled, theme.Enabled.TextColor, theme.Disabled.TextColor),
 				TextTransparency = self.props.transparency,

plugin/src/App/StatusPages/Confirming.lua

@@ -1,5 +1,3 @@
-local TextService = game:GetService("TextService")
-
 local Rojo = script:FindFirstAncestor("Rojo")
 local Plugin = Rojo.Plugin
 local Packages = Rojo.Packages

plugin/src/App/Theme.lua

@@ -19,7 +19,6 @@ local Rojo = script:FindFirstAncestor("Rojo")
 local Packages = Rojo.Packages
 
 local Roact = require(Packages.Roact)
-local Log = require(Packages.Log)
 
 local strict = require(script.Parent.Parent.strict)
 

plugin/src/InstanceMap.lua

@@ -113,27 +113,29 @@ end
 function InstanceMap:destroyInstance(instance)
 	local id = self.fromInstances[instance]
 
+	local descendants = instance:GetDescendants()
+	instance:Destroy()
+
+	-- After the instance is successfully destroyed,
+	-- we can remove all the id mappings
+
 	if id ~= nil then
 		self:removeId(id)
 	end
 
-	for _, descendantInstance in ipairs(instance:GetDescendants()) do
+	for _, descendantInstance in descendants do
 		self:removeInstance(descendantInstance)
 	end
-
-	instance:Destroy()
 end
 
 function InstanceMap:destroyId(id)
 	local instance = self.fromIds[id]
-	self:removeId(id)
-
 	if instance ~= nil then
-		for _, descendantInstance in ipairs(instance:GetDescendants()) do
-			self:removeInstance(descendantInstance)
-		end
-
-		instance:Destroy()
+		self:destroyInstance(instance)
+	else
+		-- There is no instance with this id, so we can just remove the id
+		-- without worrying about instance destruction
+		self:removeId(id)
 	end
 end
 

plugin/src/PatchTree.lua

@@ -426,22 +426,71 @@ function PatchTree.updateMetadata(tree, patch, instanceMap, unappliedPatch)
 	-- Update isWarning metadata
 	for _, failedChange in unappliedPatch.updated do
 		local node = tree:getNode(failedChange.id)
-		if node then
-			node.isWarning = true
-			Log.trace("Marked node as warning: {} {}", node.id, node.name)
-
-			if node.changeList then
-				for _, change in node.changeList do
-					if failedChange.changedProperties[change[1]] then
-						Log.trace("  Marked property as warning: {}", change[1])
-						if change[4] == nil then
-							change[4] = {}
-						end
-						change[4].isWarning = true
-					end
-				end
-			end
+		if not node then
+			continue
 		end
+
+		node.isWarning = true
+		Log.trace("Marked node as warning: {} {}", node.id, node.name)
+
+		if not node.changeList then
+			continue
+		end
+		for _, change in node.changeList do
+			local property = change[1]
+			local propertyFailedToApply = if property == "Name"
+				then failedChange.changedName ~= nil -- Name is not in changedProperties, so it needs a special case
+				else failedChange.changedProperties[property] ~= nil
+
+			if not propertyFailedToApply then
+				-- This change didn't fail, no need to mark
+				continue
+			end
+			if change[4] == nil then
+				change[4] = { isWarning = true }
+			else
+				change[4].isWarning = true
+			end
+			Log.trace("  Marked property as warning: {}.{}", node.name, property)
+		end
+	end
+	for failedAdditionId in unappliedPatch.added do
+		local node = tree:getNode(failedAdditionId)
+		if not node then
+			continue
+		end
+
+		node.isWarning = true
+		Log.trace("Marked node as warning: {} {}", node.id, node.name)
+
+		if not node.changeList then
+			continue
+		end
+		for _, change in node.changeList do
+			-- Failed addition means that all properties failed to be added
+			if change[4] == nil then
+				change[4] = { isWarning = true }
+			else
+				change[4].isWarning = true
+			end
+			Log.trace("  Marked property as warning: {}.{}", node.name, change[1])
+		end
+	end
+	for _, failedRemovalIdOrInstance in unappliedPatch.removed do
+		local failedRemovalId = if Types.RbxId(failedRemovalIdOrInstance)
+			then failedRemovalIdOrInstance
+			else instanceMap.fromInstances[failedRemovalIdOrInstance]
+		if not failedRemovalId then
+			continue
+		end
+
+		local node = tree:getNode(failedRemovalId)
+		if not node then
+			continue
+		end
+
+		node.isWarning = true
+		Log.trace("Marked node as warning: {} {}", node.id, node.name)
 	end
 
 	-- Update if instances exist

plugin/src/Reconciler/applyPatch.lua

@@ -25,10 +25,15 @@ local function applyPatch(instanceMap, patch)
 	local unappliedPatch = PatchSet.newEmpty()
 
 	for _, removedIdOrInstance in ipairs(patch.removed) do
-		if Types.RbxId(removedIdOrInstance) then
-			instanceMap:destroyId(removedIdOrInstance)
-		else
-			instanceMap:destroyInstance(removedIdOrInstance)
+		local removeInstanceSuccess = pcall(function()
+			if Types.RbxId(removedIdOrInstance) then
+				instanceMap:destroyId(removedIdOrInstance)
+			else
+				instanceMap:destroyInstance(removedIdOrInstance)
+			end
+		end)
+		if not removeInstanceSuccess then
+			table.insert(unappliedPatch.removed, removedIdOrInstance)
 		end
 	end
 
@@ -170,7 +175,13 @@ local function applyPatch(instanceMap, patch)
 		end
 
 		if update.changedName ~= nil then
-			instance.Name = update.changedName
+			local setNameSuccess = pcall(function()
+				instance.Name = update.changedName
+			end)
+			if not setNameSuccess then
+				unappliedUpdate.changedName = update.changedName
+				partiallyApplied = true
+			end
 		end
 
 		if update.changedMetadata ~= nil then
@@ -183,15 +194,15 @@ local function applyPatch(instanceMap, patch)
 
 		if update.changedProperties ~= nil then
 			for propertyName, propertyValue in pairs(update.changedProperties) do
-				local ok, decodedValue = decodeValue(propertyValue, instanceMap)
-				if not ok then
+				local decodeSuccess, decodedValue = decodeValue(propertyValue, instanceMap)
+				if not decodeSuccess then
 					unappliedUpdate.changedProperties[propertyName] = propertyValue
 					partiallyApplied = true
 					continue
 				end
 
-				local ok = setProperty(instance, propertyName, decodedValue)
-				if not ok then
+				local setPropertySuccess = setProperty(instance, propertyName, decodedValue)
+				if not setPropertySuccess then
 					unappliedUpdate.changedProperties[propertyName] = propertyValue
 					partiallyApplied = true
 				end

plugin/src/Reconciler/decodeValue.lua

@@ -27,9 +27,9 @@ local function decodeValue(encodedValue, instanceMap)
 		end
 	end
 
-	local ok, decodedValue = RbxDom.EncodedValue.decode(encodedValue)
+	local decodeSuccess, decodedValue = RbxDom.EncodedValue.decode(encodedValue)
 
-	if not ok then
+	if not decodeSuccess then
 		return false,
 			Error.new(Error.CannotDecodeValue, {
 				encodedValue = encodedValue,

plugin/src/Reconciler/diff.lua

@@ -147,13 +147,13 @@ local function diff(instanceMap, virtualInstances, rootId)
 
 		local changedProperties = {}
 		for propertyName, virtualValue in pairs(virtualInstance.Properties) do
-			local ok, existingValueOrErr = getProperty(instance, propertyName)
+			local getProperySuccess, existingValueOrErr = getProperty(instance, propertyName)
 
-			if ok then
+			if getProperySuccess then
 				local existingValue = existingValueOrErr
-				local ok, decodedValue = decodeValue(virtualValue, instanceMap)
+				local decodeSuccess, decodedValue = decodeValue(virtualValue, instanceMap)
 
-				if ok then
+				if decodeSuccess then
 					if not trueEquals(existingValue, decodedValue) then
 						Log.debug(
 							"{}.{} changed from '{}' to '{}'",
@@ -165,7 +165,6 @@ local function diff(instanceMap, virtualInstances, rootId)
 						changedProperties[propertyName] = virtualValue
 					end
 				else
-					local propertyType = next(virtualValue)
 					Log.warn(
 						"Failed to decode property {}.{}. Encoded property was: {:#?}",
 						virtualInstance.ClassName,
@@ -178,10 +177,8 @@ local function diff(instanceMap, virtualInstances, rootId)
 
 				if err.kind == Error.UnknownProperty then
 					Log.trace("Skipping unknown property {}.{}", err.details.className, err.details.propertyName)
-				elseif err.kind == Error.UnreadableProperty then
-					Log.trace("Skipping unreadable property {}.{}", err.details.className, err.details.propertyName)
 				else
-					return false, err
+					Log.trace("Skipping unreadable property {}.{}", err.details.className, err.details.propertyName)
 				end
 			end
 		end
@@ -220,9 +217,9 @@ local function diff(instanceMap, virtualInstances, rootId)
 					table.insert(patch.removed, childInstance)
 				end
 			else
-				local ok, err = diffInternal(childId)
+				local diffSuccess, err = diffInternal(childId)
 
-				if not ok then
+				if not diffSuccess then
 					return false, err
 				end
 			end
@@ -243,9 +240,9 @@ local function diff(instanceMap, virtualInstances, rootId)
 		return true
 	end
 
-	local ok, err = diffInternal(rootId)
+	local diffSuccess, err = diffInternal(rootId)
 
-	if not ok then
+	if not diffSuccess then
 		return false, err
 	end
 

plugin/src/Reconciler/hydrate.lua

@@ -31,13 +31,13 @@ local function hydrate(instanceMap, virtualInstances, rootId, rootInstance)
 				-- We guard accessing Name and ClassName in order to avoid
 				-- tripping over children of DataModel that Rojo won't have
 				-- permissions to access at all.
-				local ok, name, className = pcall(function()
+				local accessSuccess, name, className = pcall(function()
 					return childInstance.Name, childInstance.ClassName
 				end)
 
 				-- This rule is very conservative and could be loosened in the
 				-- future, or more heuristics could be introduced.
-				if ok and name == virtualChild.Name and className == virtualChild.ClassName then
+				if accessSuccess and name == virtualChild.Name and className == virtualChild.ClassName then
 					isExistingChildVisited[childIndex] = true
 					hydrate(instanceMap, virtualInstances, childId, childInstance)
 					break

plugin/src/Reconciler/reify.lua

@@ -53,9 +53,9 @@ function reifyInner(instanceMap, virtualInstances, id, parentInstance, unapplied
 	-- Instance.new can fail if we're passing in something that can't be
 	-- created, like a service, something enabled with a feature flag, or
 	-- something that requires higher security than we have.
-	local ok, instance = pcall(Instance.new, virtualInstance.ClassName)
+	local createSuccess, instance = pcall(Instance.new, virtualInstance.ClassName)
 
-	if not ok then
+	if not createSuccess then
 		addAllToPatch(unappliedPatch, virtualInstances, id)
 		return
 	end
@@ -80,14 +80,14 @@ function reifyInner(instanceMap, virtualInstances, id, parentInstance, unapplied
 			continue
 		end
 
-		local ok, value = decodeValue(virtualValue, instanceMap)
-		if not ok then
+		local decodeSuccess, value = decodeValue(virtualValue, instanceMap)
+		if not decodeSuccess then
 			unappliedProperties[propertyName] = virtualValue
 			continue
 		end
 
-		local ok = setProperty(instance, propertyName, value)
-		if not ok then
+		local setPropertySuccess = setProperty(instance, propertyName, value)
+		if not setPropertySuccess then
 			unappliedProperties[propertyName] = virtualValue
 		end
 	end
@@ -148,8 +148,8 @@ function applyDeferredRefs(instanceMap, deferredRefs, unappliedPatch)
 			continue
 		end
 
-		local ok = setProperty(entry.instance, entry.propertyName, targetInstance)
-		if not ok then
+		local setPropertySuccess = setProperty(entry.instance, entry.propertyName, targetInstance)
+		if not setPropertySuccess then
 			markFailed(entry.id, entry.propertyName, entry.virtualValue)
 		end
 	end

plugin/src/Reconciler/reify.spec.lua

@@ -3,7 +3,6 @@ return function()
 
 	local PatchSet = require(script.Parent.Parent.PatchSet)
 	local InstanceMap = require(script.Parent.Parent.InstanceMap)
-	local Error = require(script.Parent.Error)
 
 	local function isEmpty(table)
 		return next(table) == nil, "Table was not empty"

plugin/src/Reconciler/setProperty.lua

@@ -28,9 +28,9 @@ local function setProperty(instance, propertyName, value)
 			})
 	end
 
-	local ok, err = descriptor:write(instance, value)
+	local writeSuccess, err = descriptor:write(instance, value)
 
-	if not ok then
+	if not writeSuccess then
 		if err.kind == RbxDom.Error.Kind.Roblox and err.extra:find("lacking permission") then
 			return false,
 				Error.new(Error.LackingPropertyPermissions, {

plugin/src/ServeSession.lua

@@ -21,8 +21,8 @@ local Status = strict("Session.Status", {
 	Disconnected = "Disconnected",
 })
 
-local function debugPatch(patch)
-	return Fmt.debugify(patch, function(patch, output)
+local function debugPatch(object)
+	return Fmt.debugify(object, function(patch, output)
 		output:writeLine("Patch {{")
 		output:indent()
 
@@ -197,7 +197,7 @@ function ServeSession:__onActiveScriptChanged(activeScript)
 		local existingParent = activeScript.Parent
 		activeScript.Parent = nil
 
-		for i = 1, 3 do
+		for _ = 1, 3 do
 			RunService.Heartbeat:Wait()
 		end
 
@@ -251,7 +251,10 @@ function ServeSession:__initialSync(serverInfo)
 
 		if userDecision == "Abort" then
 			return Promise.reject("Aborted Rojo sync operation")
-		elseif userDecision == "Reject" and self.__twoWaySync then
+		elseif userDecision == "Reject" then
+			if not self.__twoWaySync then
+				return Promise.reject("Cannot reject sync operation without two-way sync enabled")
+			end
 			-- The user wants their studio DOM to write back to their Rojo DOM
 			-- so we will reverse the patch and send it back
 
@@ -268,7 +271,7 @@ function ServeSession:__initialSync(serverInfo)
 				table.insert(inversePatch.updated, update)
 			end
 			-- Add the removed instances back to Rojo
-			-- selene:allow(empty_if, unused_variable)
+			-- selene:allow(empty_if, unused_variable, empty_loop)
 			for _, instance in catchUpPatch.removed do
 				-- TODO: Generate ID for our instance and add it to inversePatch.added
 			end
@@ -277,7 +280,7 @@ function ServeSession:__initialSync(serverInfo)
 				table.insert(inversePatch.removed, id)
 			end
 
-			self.__apiContext:write(inversePatch)
+			return self.__apiContext:write(inversePatch)
 		elseif userDecision == "Accept" then
 			local unappliedPatch = self.__reconciler:applyPatch(catchUpPatch)
 
@@ -287,6 +290,10 @@ function ServeSession:__initialSync(serverInfo)
 					PatchSet.humanSummary(self.__instanceMap, unappliedPatch)
 				)
 			end
+
+			return Promise.resolve()
+		else
+			return Promise.reject("Invalid user decision: " .. userDecision)
 		end
 	end)
 end

plugin/src/createSignal.lua

@@ -1,53 +0,0 @@
---[[
-	Create a new signal that can be connected to, disconnected from, and fired.
-
-	Usage:
-
-		local signal = createSignal()
-		local disconnect = signal:connect(function(...)
-			print("fired:", ...)
-		end)
-
-		signal:fire("a", "b", "c")
-		disconnect()
-
-	Avoids mutating listeners list directly to prevent iterator invalidation if
-	a listener is disconnected while the signal is firing.
-]]
-local function createSignal()
-	local listeners = {}
-
-	local function connect(newListener)
-		local nextListeners = {}
-		for listener in pairs(listeners) do
-			nextListeners[listener] = true
-		end
-
-		nextListeners[newListener] = true
-		listeners = nextListeners
-
-		return function()
-			local nextListeners = {}
-			for listener in pairs(listeners) do
-				if listener ~= newListener then
-					nextListeners[listener] = true
-				end
-			end
-
-			listeners = nextListeners
-		end
-	end
-
-	local function fire(...)
-		for listener in pairs(listeners) do
-			listener(...)
-		end
-	end
-
-	return {
-		connect = connect,
-		fire = fire,
-	}
-end
-
-return createSignal