feat: add connection duplication - #4371
Conversation
📝 WalkthroughWalkthroughAdds connection duplication across the instance controller, TRPC API, and web connection list. Duplicates copy configuration and ordering while excluding source control state, and the new UI action configures the duplicated connection. ChangesConnection duplication
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f397b19f-8d69-4d9a-84ea-7f143a6587b4
📒 Files selected for processing (5)
companion/lib/Instance/Connection/TrpcRouter.tscompanion/lib/Instance/Controller.tscompanion/test/Instance/Connection/Duplicate.test.tswebui/src/Connections/ConnectionList/ConnectionsTableRow.tsxwebui/src/Connections/ConnectionList/__tests__/ConnectionsTableRow.test.tsx
|
My issue with this is that it would be a breaking feature for some modules. For example the Twitch module uses the OAuth Device Code Flow, so creating a duplicate connection would break the first. Similarly there are modules that connect to devices/services that only allow a limited number of concurrent connections in which case duplicating the connection with the same config could either cause the new connection to error or potentially disconnect the original connection. |
|
Thanks for pointing this out. I overlooked the module-specific side effects of copying credentials and immediately starting the duplicated connection. Would creating duplicates in a disabled state be sufficient, or should duplication require explicit module support? I’m happy to revise the implementation based on your preferred direction. |
|
Duplicated connections starting in a disabled state would help with some, such as those that connect to devices/services that have limitations on concurrent connections, but for others such as Twitch as I mentioned there's no suitable way to handle duplication as even if the duplicate connection is in a disabled state when you enable it the original connection will effectively be killed and unusable until the user goes through the entire authentication process again which completely negates the point of duplicating the connection. IMO I would suggest an update to the module API that would allow module developers to control if their module can or can't be duplicated, as that way the user will then only have the option to duplicate connections that is safe to do so and can't accidently duplicate a connection that would kill the original because they are unaware of the technical side of things. |
|
What if this was done on the ui side? It could then show a config form for the new connection (driven by the config fields of the old connection), allowing it to be edited before being created. I don't know how to handle disabled connections for this though, we won't have any known config fields for them..
It also means it would be an incredibly slow rollout of the feature. While I am against doing this if we need to, it would be nice to find a way to avoid this. Also worth noting that is roughly the same as the number of new modules. I dont know what adoption of 2.0 in older modules, but some new ones aren't 2.0 because Buttons doesn't yet support api 2.0 |
That would help with the Twitch module, as I store OAuth tokens in the config but they don't have an associated config field. This would also help with certain modules that download several MB of data from devices and store that in a connection config, as there's no sense duplicating all that data along with the connection if it's just going to be pointed to a completely separate device so would need fresh data anyway. One other thing I'm curious about is how much use this feature really will be? If your duplicating connections then chances are you are going to need to edit the config of them anyway, such as a camera connection that's duplicated you still need to edit the config to point to a different camera. How often do users need to duplicate a connection and not need to edit the config? If the goal of this PR is to reduce the number of clicks to add a new connection then perhaps a change there would be more appropriate (for example currently when you add a connection it goes to that new connections config, but if there was a 'add multiple connections' button you could create multiple connections without needing to go back to the list and find the module each time. As either way doing it like that, or duplicating an existing connection, you're likely going to have to edit the connection config either way so at least creating multiple new connections would not have the same issues as duplicating existing ones. |
Or perhaps an 'add another' instead of duplicate? But I would say that having a duplicate means that for the bunch of fields which dont need changing (port number, perhaps some common credentials, any module functionality toggles) being able to duplicate can avoid the repetition of configuring from scratch (or checking what the old one was set to).
Or maybe there is? If that data is a definition of the functionality of the device, and is needed for actions/feedbacks to exist, preserving this means they can do an initial program offline. |
|
Oh, it is also worth noting that Buttons already has a duplicate button, so it would be good to have a solution that we could recommend to them (which isnt simply don't allow it unless the module has opted into it) |
Would an opt-out be better? so by default all modules regardless of API version can be duplicated, but modules that wish to opt-out could download a new API version that provides a flag on the instance to disable it. The negative impact then would be limited to Companion users on a version of Companion that has duplication while also running a module version that has not updated to disable it should they need to. This also allows for all modules, even those no longer maintained, to be duplicated. |
|
That makes sense to me. |
Summary
The duplication is owned by the backend because connection config and secrets are intentionally not exposed in the connection-list client model. It reuses the existing connection creation, configuration, and reorder paths. The duplicate is created disabled, fully configured and positioned, and only then enabled when the source was enabled, so it cannot start with the temporary default config.
For legacy connections whose module version is
null, duplication follows the existing add-connection behavior and resolves the latest installed version.Validation
yarn install --immutableyarn build:tsyarn build:graphics-typesyarn prettier --check .yarn lint(no errors; six existing warnings)yarn check-typesyarn workspace @companion-app/webui buildCloses #4034
AI assistance was used for repository research, implementation, and test preparation. The resulting changes and validation output were reviewed locally.
Summary by CodeRabbit
New Features
Bug Fixes
Tests