feat: Implement copy paste functionality for feedbacks/actions#4083
feat: Implement copy paste functionality for feedbacks/actions#4083njibhu wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR introduces entity clipboard functionality, enabling users to copy and paste entities throughout the control system. Changes include a new backend paste API endpoint, a MobX-backed frontend clipboard store, integration with the entity editor service, and UI controls for copy/paste operations in entity panels. Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
webui/src/Controls/Components/AddEntityPanel.tsx (1)
52-58: Consider if clipboard should clear after paste (design decision)The clipboard isn't cleared after a successful paste, which means users can paste the same entity multiple times. This is actually similar to how OS clipboards work and could be a nice feature for duplicating the same action/feedback to multiple buttons!
Just wanted to flag this as a conscious design choice. If you'd prefer "cut-style" behavior (clear after paste), you could call
entityClipboard.clear()after a successful paste. But the current approach seems like a reasonable UX for your use case of "large surfaces with many similar buttons" mentioned in the PR description.webui/src/Stores/EntityClipboardStore.ts (1)
7-19: Consider marking getters ascomputedfor MobX optimizationThe getters work correctly as-is, but MobX can cache computed values and only recalculate when their dependencies change. This is a minor optimization, but it's a nice-to-have for MobX best practices.
💡 Optional improvement to add computed decorators
constructor() { makeObservable<EntityClipboardStore, '_copiedEntity'>(this, { _copiedEntity: observable, + copiedEntity: computed, + copiedEntityType: computed, }) }You'll also need to import
computed:-import { action, makeObservable, observable } from 'mobx' +import { action, computed, makeObservable, observable } from 'mobx'
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72451ea5-85f6-42d6-8796-4425ce50686e
📒 Files selected for processing (11)
companion/lib/Controls/Entities/EntityInstance.tscompanion/lib/Controls/Entities/EntityListPoolBase.tscompanion/lib/Controls/EntitiesTrpcRouter.tsshared-lib/lib/Model/EntityModel.tswebui/src/ContextData.tsxwebui/src/Controls/Components/AddEntityPanel.tsxwebui/src/Controls/Components/EntityCellControls.tsxwebui/src/Services/Controls/ControlActionsService.tswebui/src/Services/Controls/ControlEntitiesService.tswebui/src/Stores/EntityClipboardStore.tswebui/src/Stores/RootAppStore.tsx
| performPaste: (_ownerId: EntityOwner | null, _entities: SomeEntityModel[]) => { | ||
| // Not supported in action recorder | ||
| }, |
There was a problem hiding this comment.
Paste is currently exposed as a no-op in action recorder.
On Line 71, this handler does nothing, so users can hit “Paste” and get no effect. For unsupported features in this service, it’s better to keep the action unavailable (or surface an explicit “not supported” notification).
Suggested fix
- performPaste: (_ownerId: EntityOwner | null, _entities: SomeEntityModel[]) => {
- // Not supported in action recorder
- },
+ performPaste: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| performPaste: (_ownerId: EntityOwner | null, _entities: SomeEntityModel[]) => { | |
| // Not supported in action recorder | |
| }, | |
| performPaste: undefined, |
There was a problem hiding this comment.
I didn't want to break with what seems to be the existing convention, all other functions that are not supported in that file are implemented like this.
There was a problem hiding this comment.
@njibhu, that's a fair point! Looking at the file more carefully, the convention here is indeed mixed — performLearn, setEnabled, and setHeadline are set to undefined, while setInverted, setVariableName, setVariableValue, and a few others use the no-op pattern you followed. Both approaches exist side by side, so your choice is completely reasonable. I'll defer to your judgment on keeping it consistent with the no-op style you were following. Thanks for explaining! 🐇
🧠 Learnings used
Learnt from: arikorn
Repo: bitfocus/companion PR: 3916
File: webui/src/Hooks/useShowSecondaryPanel.ts:27-29
Timestamp: 2026-03-24T00:26:41.745Z
Learning: In the bitfocus/companion repo’s TanStack Router configuration, setting `trailingSlash: 'never'` on `createRouter()` does not normalize/strip trailing slashes at runtime (it had no effect in testing). When reviewing route-matching issues related to trailing slashes, do not recommend `trailingSlash: 'never'` as a fix in this repo; look for other causes/solutions instead.
Learnt from: Julusian
Repo: bitfocus/companion PR: 4077
File: companion/test/Service/Satellite/SatelliteApi.test.ts:650-654
Timestamp: 2026-04-08T21:40:58.021Z
Learning: In the bitfocus/companion repo, avoid adding code review comments that only restate TypeScript compiler type errors (e.g., TS18048 “possibly undefined”, type mismatches). Rely on the TypeScript compiler/CI to catch these issues, and focus review comments on problems that are not already covered by compilation (logic, correctness, maintainability, security, etc.).
There was a problem hiding this comment.
The pattern here is that ui 'features' which are optional can be set to undefined, which is used by the ui to hide any ui for that feature.
some fo the editing is no-ops because the entities here are set to readonly, so in reality these can never be fired.
so maybe the performPaste should be undefined here, to let the ui use this to hide the paste button?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
I haven't reviewed or tried the code but this is much-needed functionality. Thanks for taking it on @njibhu ! One suggestion: from your description it looks like one can only paste to the end of the actions/feedbacks list. It would be great if one could paste to any position: (1) select an existing item to replace it. (2) select the line between two items or line at top/bottom of list to insert the item. Either way, the paste location could be highlighted with a border/line, similar to the buttons grid. |
Thanks for checking this. 
|
|
@njibhu I agree: your current code provides a foundation to build upon. The good news is that most of the bits and pieces needed to add the type of flexibility I described is already in other parts of the codebase, so if you're interested, here's an outline of sorts for how to "supercharge" your PR. After thinking about it (and looking a Trello -- see image, below), I realized that, using your design, each action/feedback ("entity" i think?) needs only a single new HTML element added at its top. The element replaces the current separation and acts as a selectable marker for where to add the next entity. The cool thing is that it could apply to both "add action" and "paste." Adding an entity below the last item is the current behavior, so it's already covered. Here's the idea:
The first two steps build strictly upon what you've done. The other steps might need to make the whole action selectable in order to handle cut/copy (again, see the drag-and-drop or button-grid for example of handling highlighting) - though paste would be possible without that. Minor notes about your code:
Example of how to visually mark adding between elements (from Trello). I'm not recommending the "+" element here, just the dashed line. 
This CSS/HTML creates a "spacer" which shows a dashed line upon hover, etc. (You should be able to copy this as-is into a "test.html" file to see it work in your browser.) Colors and thicknesses are a suggestion. <html>
<head>
<style>
.spacer {
background-color: lightgrey;
display: flex;
align-items: center;
height: 10px; /* choose a height for the spacer */
width: 100%;
}
.spacer::after {
content: '';
border: 1px dashed darkred; /* this forms a 2-pixel border (top & bottom with 0 height intervening) */
height: 0px;
flex: 100%;
visibility: hidden;
}
/* in React, manage the .selected property: toggle onClick of the spacer div, for example */
.spacer:is(:hover, :focus, .selected)::after {
visibility: visible;
}
</style>
</head>
<body>
<!-- the tabindex makes it able to acquire focus, including by keyboard -->
<div class="spacer" tabindex="0"></div>
</body>
</html> |
|
I think it would be probably best to keep the discussion around the scope of the issue and the PR. @Julusian, any feedback or changes required ? |
|
while I would really like this to use the system clipboard, I think that would require the navigator clipboard api which only works over https (or off localhost). my brain isnt in a space of thinking about ui much toady, so not 100% sure how I feel about this. it does feel like we are collecting a lot of buttons, and maybe some kind of dropdown or context menu might be appropriate now/soon? but if we want to allow for multi select for bulk copy/paste/move as a follow up that will probably require a larger rethink of how these elements look or are arranged or something to allow for that, so maybe the concern of number of buttons is not good to think about before that |
So the original reason why I didn't want to use the navigator's clipboard api, was that I would see it only really be relevant if we were trying to "export/import" outside of companion (but there's already an export feature). But since this is a copy/paste action scoped to the companion instance, I do see a lot of benefits to keep it fully in-browser, which means that for example we don't need to parse unknown inputs. And as you mentioned, the clipboard api also has further restrictions, and can even be blocked fully by certain browsers/policies. I think right now supporting copying across tabs could be added without too much changes via localstorage. I can investigate this change here. |
Im not sure if local storage is best, as that will persist between restarts of the browser. but I think there is some way tabs can communicate, which could work and be good enough? |
3 participants
Implementing #4028
Adds a new copy action which copies the selected action to a clipboard:
Once a selected action is in the clipboard, a new paste action is available:
The copy pasting works across buttons, which is a very much wanted QoL to smooth out the iterative process for large surfaces with many similar buttons.
This feature also works with feedbacks.
Summary by CodeRabbit
Release Notes