rojo-rbx / rojo

Rojo enables Roblox developers to use professional-grade software engineering tools
https://rojo.space
Mozilla Public License 2.0
976 stars 182 forks source link

Disconnecting with synced changes expanded breaks the plugin #671

Closed ecurtiss closed 1 year ago

ecurtiss commented 1 year ago

Whenever I Connect > Accept > Expand synced changes > Disconnect, I receive this error

user_RojoManagedPlugin.rbxm.Rojo.Plugin.App.StatusPages.Connected:79: attempt to index nil with '__instanceMap'

with a long roact stacktrace underneath. When this happens, the plugin stops syncing and looks like this: image It does not happen when I disconnect with the synced changes minimized.

boatbomber commented 1 year ago

(Disclaimer: this is a theory based entirely on my mental model of the plugin, as I am not available to test right now)

Our plugin has really cool transitions between pages. https://github.com/rojo-rbx/rojo/blob/master/plugin/src/App/Page.lua#L24-L34

However, I forgot about that when building this feature and made a foolish assumption. You see, when you disconnect the plugin transitions between the Connected and NotConnected pages, which means that Connected is briefly being rendered even when the plugin state isn't really connected anymore.

This means that serveSession is nil, and therefore you're seeing an error when the Connected page render was indexing it without checking first. https://github.com/rojo-rbx/rojo/blob/master/plugin/src/App/StatusPages/Connected.lua#L79

boatbomber commented 1 year ago

This can be temporarily worked around by either 1) Minimizing the changes visualizer (since that component has the broken render, not rendering it avoids the issue) 2) Having the widget closed and using the keybinds (since nothing is rendering, we avoid the broken render)

boatbomber commented 1 year ago

I have fixed this issue by converting the ChangesDrawer component from functional to stateful so that it can hold onto the serveSession during its lifetime, ensuring that it can render the last known change even as it fades away.

PR coming soon.