Make "Always" confirmation behavior setting work by handling it for ongoing sync patches

- Add "Always" option to confirmationBehavior setting
- Route WebSocket patches through the confirmation callback
- Fix UI not transitioning back to Connected after confirming ongoing patches
- Fix DomLabel Flipper motor cleanup on unmount to prevent errors
This commit is contained in:
warsam 2026-01-23 19:39:29 -08:00 • committed by Joe
parent a7f36fab5e
commit a05acf8d35
7 changed files with 638 additions and 90 deletions

View file

@ -31,6 +31,7 @@ Making a new release? Simply add the new header with the version and date undern
## Unreleased
* Implemented the "Always" confirmation behavior setting, which prompts for confirmation on every sync patch, not just the initial sync. Changes that arrive during confirmation are merged into the pending patch and the UI updates in real-time. ([#1216])
* Fixed a bug caused by having reference properties (such as `ObjectValue.Value`) that point to an Instance not included in syncback. ([#1179])
* Fixed instance replacement fallback failing when too many instances needed to be replaced. ([#1192])
* Fixed a bug where MacOS paths weren't being handled correctly. ([#1201])
@ -40,6 +41,8 @@ Making a new release? Simply add the new header with the version and date undern
[#1192]: https://github.com/rojo-rbx/rojo/pull/1192
[#1201]: https://github.com/rojo-rbx/rojo/pull/1201
[#1211]: https://github.com/rojo-rbx/rojo/pull/1211
[#1215]: https://github.com/rojo-rbx/rojo/pull/1215
[#1216]: https://github.com/rojo-rbx/rojo/pull/1216
## [7.7.0-rc.1] (November 27th, 2025)

View file

@ -113,6 +113,8 @@ end
function DomLabel:willUnmount()
self.isMounted = false
-- Stop the motor to prevent onStep callbacks after unmount
self.motor:stop()
if self.motorStepConnection then
self.motorStepConnection:disconnect()
self.motorStepConnection = nil

View file

@ -721,6 +721,7 @@ function App:startSession()
end
local confirmationBehavior = Settings:get("confirmationBehavior")
if confirmationBehavior ~= "Always" then
if confirmationBehavior == "Initial" then
-- Only confirm if we haven't synced this project yet this session
if self.knownProjects[serverInfo.projectName] then
@ -752,6 +753,7 @@ function App:startSession()
Log.trace("Accepting patch without confirmation because behavior is set to Never")
return "Accept"
end
end
-- The datamodel name gets overwritten by Studio, making confirmation of it intrusive
-- and unnecessary. This special case allows it to be accepted without confirmation.
@ -791,7 +793,36 @@ function App:startSession()
timeout = 7,
})
return self.confirmationEvent:Wait()
local result = self.confirmationEvent:Wait()
-- Reset UI state back to Connected after confirmation
-- This is needed for ongoing WebSocket patches where the session
-- is already connected and won't trigger a status change
if self.serveSession and self.serveSession:getStatus() == ServeSession.Status.Connected then
self:setState({
appStatus = AppStatus.Connected,
toolbarIcon = Assets.Images.PluginButtonConnected,
-- Clear patchTree to avoid animation issues when the
-- PatchVisualizer unmounts while Flipper motors are running
patchTree = nil,
})
end
return result
end)
serveSession:setPatchUpdateCallback(function(instanceMap, patch)
-- If all changes have been reverted, auto-accept the empty patch
if PatchSet.isEmpty(patch) then
Log.trace("Patch became empty after merging, auto-accepting")
self.confirmationBindable:Fire("Accept")
return
end
-- Update the patchTree when new changes arrive during confirmation
self:setState({
patchTree = PatchTree.build(patch, instanceMap, { "Property", "Current", "Incoming" }),
})
end)
serveSession:start()

View file

@ -7,6 +7,7 @@ local Packages = script.Parent.Parent.Packages
local t = require(Packages.t)
local Types = require(script.Parent.Types)
local decodeValue = require(script.Parent.Reconciler.decodeValue)
local function deepEqual(a: any, b: any): boolean
local typeA = typeof(a)
@ -282,6 +283,101 @@ function PatchSet.assign(target, ...)
return target
end
--[[
Merge a source PatchSet into the target, properly reconciling conflicts.
Unlike assign(), this handles cases where a removal cancels out an addition,
or where an update supersedes a previous update to the same instance.
If instanceMap is provided, updates that revert to the current instance state
will be removed from the patch entirely.
]]
function PatchSet.merge(target, source, instanceMap)
-- Process removals: if we're removing something that was added, just delete the addition
for _, removed in ipairs(source.removed) do
if target.added[removed] ~= nil then
-- This removal cancels out a previous addition
target.added[removed] = nil
else
table.insert(target.removed, removed)
end
end
-- Process additions
for id, added in pairs(source.added) do
target.added[id] = added
end
-- Process updates: merge with existing updates to the same instance
for _, update in ipairs(source.updated) do
local existingIndex = nil
for i, existing in ipairs(target.updated) do
if existing.id == update.id then
existingIndex = i
break
end
end
if existingIndex then
-- Merge the updates
local existing = target.updated[existingIndex]
if update.changedName ~= nil then
existing.changedName = update.changedName
end
if update.changedClassName ~= nil then
existing.changedClassName = update.changedClassName
end
if update.changedMetadata ~= nil then
existing.changedMetadata = update.changedMetadata
end
for prop, value in pairs(update.changedProperties) do
existing.changedProperties[prop] = value
end
-- Check if this update now matches current instance state (i.e., reverted)
-- If so, remove properties that match or remove the update entirely
if instanceMap then
local instance = instanceMap.fromIds[existing.id]
if instance then
-- Check if name change matches current
if existing.changedName ~= nil and existing.changedName == instance.Name then
existing.changedName = nil
end
-- Check if class change matches current
if existing.changedClassName ~= nil and existing.changedClassName == instance.ClassName then
existing.changedClassName = nil
end
-- Check each changed property against current instance value
for propName, encodedValue in pairs(existing.changedProperties) do
local decodeSuccess, decodedValue = decodeValue(encodedValue, instanceMap)
if decodeSuccess then
local currentSuccess, currentValue = pcall(function()
return instance[propName]
end)
if currentSuccess and deepEqual(decodedValue, currentValue) then
existing.changedProperties[propName] = nil
end
end
end
-- If no changes remain, remove the update entirely
local hasChanges = existing.changedName ~= nil
or existing.changedClassName ~= nil
or existing.changedMetadata ~= nil
or next(existing.changedProperties) ~= nil
if not hasChanges then
table.remove(target.updated, existingIndex)
end
end
end
else
table.insert(target.updated, update)
end
end
return target
end
function PatchSet.addedIdList(patchSet): { string }
local idList = table.create(#patchSet.added)
for id in patchSet.added do

View file

@ -0,0 +1,341 @@
return function()
local PatchSet = require(script.Parent.PatchSet)
local InstanceMap = require(script.Parent.InstanceMap)
describe("newEmpty", function()
it("should create an empty patch", function()
local patch = PatchSet.newEmpty()
expect(PatchSet.isEmpty(patch)).to.equal(true)
end)
end)
describe("isEmpty", function()
it("should return true for empty patches", function()
local patch = PatchSet.newEmpty()
expect(PatchSet.isEmpty(patch)).to.equal(true)
end)
it("should return false when patch has removals", function()
local patch = PatchSet.newEmpty()
table.insert(patch.removed, "some-id")
expect(PatchSet.isEmpty(patch)).to.equal(false)
end)
it("should return false when patch has additions", function()
local patch = PatchSet.newEmpty()
patch.added["some-id"] = { Id = "some-id", ClassName = "Folder", Name = "Test" }
expect(PatchSet.isEmpty(patch)).to.equal(false)
end)
it("should return false when patch has updates", function()
local patch = PatchSet.newEmpty()
table.insert(patch.updated, { id = "some-id", changedProperties = {} })
expect(PatchSet.isEmpty(patch)).to.equal(false)
end)
end)
describe("merge", function()
it("should merge additions from source into target", function()
local target = PatchSet.newEmpty()
local source = PatchSet.newEmpty()
source.added["CHILD"] = {
Id = "CHILD",
ClassName = "Folder",
Name = "Child",
Parent = "ROOT",
Children = {},
Properties = {},
}
PatchSet.merge(target, source)
expect(target.added["CHILD"]).to.be.ok()
expect(target.added["CHILD"].Name).to.equal("Child")
end)
it("should merge removals from source into target", function()
local target = PatchSet.newEmpty()
local source = PatchSet.newEmpty()
table.insert(source.removed, "CHILD")
PatchSet.merge(target, source)
expect(#target.removed).to.equal(1)
expect(target.removed[1]).to.equal("CHILD")
end)
it("should cancel additions when source removes them", function()
local target = PatchSet.newEmpty()
target.added["CHILD"] = {
Id = "CHILD",
ClassName = "Folder",
Name = "Child",
Parent = "ROOT",
Children = {},
Properties = {},
}
local source = PatchSet.newEmpty()
table.insert(source.removed, "CHILD")
PatchSet.merge(target, source)
-- The addition should be cancelled
expect(target.added["CHILD"]).to.equal(nil)
-- And no removal should be added since it was never actually applied
expect(#target.removed).to.equal(0)
end)
it("should merge updates from source into target", function()
local target = PatchSet.newEmpty()
local source = PatchSet.newEmpty()
table.insert(source.updated, {
id = "INSTANCE",
changedName = "NewName",
changedProperties = {},
})
PatchSet.merge(target, source)
expect(#target.updated).to.equal(1)
expect(target.updated[1].changedName).to.equal("NewName")
end)
it("should combine updates for the same instance", function()
local target = PatchSet.newEmpty()
table.insert(target.updated, {
id = "INSTANCE",
changedName = "FirstName",
changedProperties = {
Value = { String = "First" },
},
})
local source = PatchSet.newEmpty()
table.insert(source.updated, {
id = "INSTANCE",
changedName = "SecondName",
changedProperties = {
OtherValue = { String = "Second" },
},
})
PatchSet.merge(target, source)
-- Should still only have one update entry
expect(#target.updated).to.equal(1)
-- Name should be overwritten by the newer update
expect(target.updated[1].changedName).to.equal("SecondName")
-- Both properties should be present
expect(target.updated[1].changedProperties.Value).to.be.ok()
expect(target.updated[1].changedProperties.OtherValue).to.be.ok()
end)
it("should remove update when reverted to current instance state", function()
-- Create an instance to compare against
local testInstance = Instance.new("StringValue")
testInstance.Name = "OriginalName"
testInstance.Value = "OriginalValue"
local instanceMap = InstanceMap.new()
instanceMap:insert("INSTANCE", testInstance)
-- Target has an update changing the name
local target = PatchSet.newEmpty()
table.insert(target.updated, {
id = "INSTANCE",
changedName = "ChangedName",
changedProperties = {},
})
-- Source reverts the name back to original
local source = PatchSet.newEmpty()
table.insert(source.updated, {
id = "INSTANCE",
changedName = "OriginalName",
changedProperties = {},
})
PatchSet.merge(target, source, instanceMap)
-- The update should be removed entirely since it matches current state
expect(#target.updated).to.equal(0)
testInstance:Destroy()
instanceMap:stop()
end)
it("should remove individual property changes when reverted", function()
-- Create an instance to compare against
local testInstance = Instance.new("StringValue")
testInstance.Name = "Test"
testInstance.Value = "OriginalValue"
local instanceMap = InstanceMap.new()
instanceMap:insert("INSTANCE", testInstance)
-- Target has property change
local target = PatchSet.newEmpty()
table.insert(target.updated, {
id = "INSTANCE",
changedName = "NewName",
changedProperties = {
Value = { String = "ChangedValue" },
},
})
-- Source reverts the property back to original
local source = PatchSet.newEmpty()
table.insert(source.updated, {
id = "INSTANCE",
changedProperties = {
Value = { String = "OriginalValue" },
},
})
PatchSet.merge(target, source, instanceMap)
-- Should still have update for the name change
expect(#target.updated).to.equal(1)
expect(target.updated[1].changedName).to.equal("NewName")
-- But the Value property should be removed since it matches current
expect(target.updated[1].changedProperties.Value).to.equal(nil)
testInstance:Destroy()
instanceMap:stop()
end)
it("should keep updates for different instances separate", function()
local target = PatchSet.newEmpty()
table.insert(target.updated, {
id = "INSTANCE_A",
changedName = "NameA",
changedProperties = {},
})
local source = PatchSet.newEmpty()
table.insert(source.updated, {
id = "INSTANCE_B",
changedName = "NameB",
changedProperties = {},
})
PatchSet.merge(target, source)
expect(#target.updated).to.equal(2)
end)
end)
describe("assign", function()
it("should merge multiple patches additively", function()
local target = PatchSet.newEmpty()
local source1 = PatchSet.newEmpty()
source1.added["A"] = { Id = "A", ClassName = "Folder", Name = "A" }
local source2 = PatchSet.newEmpty()
source2.added["B"] = { Id = "B", ClassName = "Folder", Name = "B" }
PatchSet.assign(target, source1, source2)
expect(target.added["A"]).to.be.ok()
expect(target.added["B"]).to.be.ok()
end)
end)
describe("countChanges", function()
it("should count property changes in additions", function()
local patch = PatchSet.newEmpty()
patch.added["A"] = {
Id = "A",
ClassName = "StringValue",
Name = "A",
Properties = {
Value = { String = "test" },
MaxValue = { Float64 = 100 },
},
}
expect(PatchSet.countChanges(patch)).to.equal(2)
end)
it("should count removals as single changes", function()
local patch = PatchSet.newEmpty()
table.insert(patch.removed, "A")
table.insert(patch.removed, "B")
expect(PatchSet.countChanges(patch)).to.equal(2)
end)
it("should count property updates", function()
local patch = PatchSet.newEmpty()
table.insert(patch.updated, {
id = "A",
changedProperties = {
Value = { String = "test" },
},
})
expect(PatchSet.countChanges(patch)).to.equal(1)
end)
it("should count name changes", function()
local patch = PatchSet.newEmpty()
table.insert(patch.updated, {
id = "A",
changedName = "NewName",
changedProperties = {},
})
expect(PatchSet.countChanges(patch)).to.equal(1)
end)
it("should count className changes", function()
local patch = PatchSet.newEmpty()
table.insert(patch.updated, {
id = "A",
changedClassName = "Model",
changedProperties = {},
})
expect(PatchSet.countChanges(patch)).to.equal(1)
end)
end)
describe("countInstances", function()
it("should count all affected instances", function()
local patch = PatchSet.newEmpty()
patch.added["A"] = { Id = "A", ClassName = "Folder", Name = "A" }
patch.added["B"] = { Id = "B", ClassName = "Folder", Name = "B" }
table.insert(patch.removed, "C")
table.insert(patch.updated, { id = "D", changedProperties = {} })
expect(PatchSet.countInstances(patch)).to.equal(4)
end)
end)
describe("isEqual", function()
it("should return true for identical patches", function()
local patchA = PatchSet.newEmpty()
patchA.added["A"] = { Id = "A", ClassName = "Folder", Name = "A" }
local patchB = PatchSet.newEmpty()
patchB.added["A"] = { Id = "A", ClassName = "Folder", Name = "A" }
expect(PatchSet.isEqual(patchA, patchB)).to.equal(true)
end)
it("should return false for different patches", function()
local patchA = PatchSet.newEmpty()
patchA.added["A"] = { Id = "A", ClassName = "Folder", Name = "A" }
local patchB = PatchSet.newEmpty()
patchB.added["B"] = { Id = "B", ClassName = "Folder", Name = "B" }
expect(PatchSet.isEqual(patchA, patchB)).to.equal(false)
end)
end)
end

View file

@ -105,6 +105,10 @@ function ServeSession.new(options)
__instanceMap = instanceMap,
__changeBatcher = changeBatcher,
__statusChangedCallback = nil,
__userConfirmCallback = nil,
__patchUpdateCallback = nil,
__serverInfo = nil,
__confirmingPatch = nil,
__connections = connections,
__precommitCallbacks = {},
__postcommitCallbacks = {},
@ -139,6 +143,10 @@ function ServeSession:setConfirmCallback(callback)
self.__userConfirmCallback = callback
end
function ServeSession:setPatchUpdateCallback(callback)
self.__patchUpdateCallback = callback
end
function ServeSession:setUpdateLoadingTextCallback(callback)
self.__updateLoadingText = callback
end
@ -189,6 +197,62 @@ function ServeSession:hookPostcommit(callback)
end
end
function ServeSession:__onWebSocketMessage(messagesPacket)
if self.__status == Status.Disconnected then
return
end
Log.debug("Received {} messages from Rojo server", #messagesPacket.messages)
-- Combine all messages into a single patch
local combinedPatch = PatchSet.newEmpty()
for _, message in messagesPacket.messages do
PatchSet.assign(combinedPatch, message)
end
-- If we're already waiting for confirmation, merge into the patch being confirmed
if self.__confirmingPatch ~= nil then
Log.trace("Already confirming, merging into current patch")
PatchSet.merge(self.__confirmingPatch, combinedPatch, self.__instanceMap)
-- Notify the UI to update the displayed patch
if self.__patchUpdateCallback ~= nil then
self.__patchUpdateCallback(self.__instanceMap, self.__confirmingPatch)
end
self.__apiContext:setMessageCursor(messagesPacket.messageCursor)
return
end
-- Set confirming patch before spawning to prevent race condition
-- where another message arrives before the spawned thread starts
self.__confirmingPatch = combinedPatch
-- Spawn a new thread to handle potentially yielding confirmation
task.spawn(function()
Log.trace("Processing WebSocket patch, callback exists: {}", self.__userConfirmCallback ~= nil)
Log.trace("ServerInfo exists: {}", self.__serverInfo ~= nil)
local userDecision = "Accept"
if self.__userConfirmCallback ~= nil then
userDecision = self.__userConfirmCallback(self.__instanceMap, combinedPatch, self.__serverInfo)
end
self.__confirmingPatch = nil
Log.trace("WebSocket patch decision: {}", userDecision)
if userDecision == "Abort" then
self:__stopInternal("Aborted Rojo sync operation")
return
elseif userDecision == "Accept" then
-- combinedPatch may have been updated with additional changes
-- that arrived while we were waiting for confirmation
self:__applyPatch(combinedPatch)
end
-- Note: "Reject" is not handled for ongoing patches, only for initial sync
end)
self.__apiContext:setMessageCursor(messagesPacket.messageCursor)
end
function ServeSession:start()
self:__setStatus(Status.Connecting)
self:setLoadingText("Connecting to server...")
@ -202,25 +266,26 @@ function ServeSession:start()
self.__syncSourceOnly = serverInfo.syncSourceOnly or false
self.__changeBatcher:setSyncSourceOnly(self.__syncSourceOnly)
return self:__initialSync(serverInfo):andThen(function()
self.__serverInfo = serverInfo
return self:__computeInitialPatch(serverInfo):andThen(function(catchUpPatch)
self:setLoadingText("Starting sync loop...")
self:__setStatus(Status.Connected, serverInfo.projectName)
self:__applyGameAndPlaceId(serverInfo)
return self.__apiContext:connectWebSocket({
-- Connect WebSocket BEFORE showing confirmation so changes
-- during confirmation can be merged into the patch
-- Note: connectWebSocket returns a Promise that only resolves when
-- the connection closes, so we don't chain .andThen() on it
self.__apiContext:connectWebSocket({
["messages"] = function(messagesPacket)
if self.__status == Status.Disconnected then
return
end
Log.debug("Received {} messages from Rojo server", #messagesPacket.messages)
for _, message in messagesPacket.messages do
self:__applyPatch(message)
end
self.__apiContext:setMessageCursor(messagesPacket.messageCursor)
self:__onWebSocketMessage(messagesPacket)
end,
})
}):catch(function(err)
if self.__status ~= Status.Disconnected then
self:__stopInternal(err)
end
end)
-- Now show confirmation and wait for user decision
return self:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo)
end)
end)
:catch(function(err)
@ -471,7 +536,7 @@ function ServeSession:__applyPatch(patch)
end
end
function ServeSession:__initialSync(serverInfo)
function ServeSession:__computeInitialPatch(serverInfo)
return self.__apiContext:read({ serverInfo.rootInstanceId }):andThen(function(readResponseBody)
-- Tell the API Context that we're up-to-date with the version of
-- the tree defined in this response.
@ -509,9 +574,18 @@ function ServeSession:__initialSync(serverInfo)
Log.trace("Computed hydration patch: {:#?}", debugPatch(catchUpPatch))
return catchUpPatch
end)
end
function ServeSession:__confirmAndApplyInitialPatch(catchUpPatch, serverInfo)
local userDecision = "Accept"
if self.__userConfirmCallback ~= nil then
-- Set confirming patch so WebSocket messages that arrive during
-- confirmation can be merged into this patch
self.__confirmingPatch = catchUpPatch
userDecision = self.__userConfirmCallback(self.__instanceMap, catchUpPatch, serverInfo)
self.__confirmingPatch = nil
end
if userDecision == "Abort" then
@ -559,12 +633,13 @@ function ServeSession:__initialSync(serverInfo)
return self.__apiContext:write(inversePatch)
elseif userDecision == "Accept" then
self:__setStatus(Status.Connected, serverInfo.projectName)
self:__applyGameAndPlaceId(serverInfo)
self:__applyPatch(catchUpPatch)
return Promise.resolve()
else
return Promise.reject("Invalid user decision: " .. userDecision)
end
end)
end
function ServeSession:__stopInternal(err)

View file

@ -20,7 +20,7 @@ local defaultSettings = {
checkForUpdates = true,
checkForPrereleases = false,
autoConnectPlaytestServer = false,
confirmationBehavior = "Initial" :: "Never" | "Initial" | "Large Changes" | "Unlisted PlaceId",
confirmationBehavior = "Initial" :: "Never" | "Initial" | "Always" | "Large Changes" | "Unlisted PlaceId",
largeChangesConfirmationThreshold = 5,
playSounds = true,
typecheckingEnabled = false,