Edit in sandbox: create or join a sandbox from a live workflow - #4901
Edit in sandbox: create or join a sandbox from a live workflow#4901elias-ba wants to merge 11 commits into
Conversation
From a live workflow, "Edit in sandbox" opens a picker to either branch a new sandbox from the current live version or join an active one, then lands the user in the sandbox editor with that workflow's trigger live and its own endpoint. The parent stays untouched until a change is promoted back (a later slice of the epic). - New channel events: list a parent's active sandboxes (with collaborators and the joinable workflow, sorted by last edited) and create-and-open a sandbox. - Creating clones the parent project, then promotes the edited workflow to live inside the sandbox; the other cloned workflows stay draft so state and triggers remain coherent. The sandbox copy stays editable because the read-only lock only applies to live workflows outside a sandbox. - A sandbox badge in the editor header. Server-side errors (permission, usage limit) surface their real message in the picker rather than a generic one.
Security Review ✅
|
|
@elias-ba I noticed that you haven't checked the box that you've done an AI self review of the code here. Have you done a Claude review? |
There was a problem hiding this comment.
This is looking exciting @elias-ba - it's a big review so strap in 🤩! I've left a few comments in the code that I think are worth addressing within this PR. One final code comment:
assets/js/collaborative-editor/components/EditInSandboxPicker.tsx:1 and test files — multi-paragraph block comments violate CLAUDE.md
CLAUDE.md: "Never write multi-paragraph docstrings or multi-line comment blocks — one short line max."
Four blocks violate this:
- EditInSandboxPicker.tsx lines 1–14 (14-line file header with 3 paragraph breaks)
- EditInSandboxPicker.tsx lines 36–39 (serverMessage function comment)
- EditInSandboxPicker.test.tsx lines 1–12
- Header.sandbox.test.tsx lines 1–12
My feedback from a UX/UI perspective on "Edit in sandbox" experience (not blocking but observational):
- It was a bit confusing seeing "Join an active sandbox" and then both sandboxes having disabled "Join" buttons. Perhaps it shouldn't show this section if there is no sandbox to join?
-
When hovering over the disabled "Join" button in the above scenario, there was no explanatory tooltip message to explain why. Sometimes it appears but after a long pause, not instantly.
-
The "Join an active sandbox" wording was a bit confusing.. it made me feel like I wasn't a part of that sandbox before. Not sure what would be better wording here.
-
What exactly are the collaborator circles for? What do they represent?
-
If entering a sandbox name is optional, our styling in the header should support long workflow/sandbox names more elegantly. Suggestions:
(1) Make the Save button grow
(2) Add ellipsis for the sandbox/workflow name when it's a certain length:
-
I think we should improve the toast message when you try to add a sandbox with a name that already exists rather than the generic toast message. It should tell the user that a sandbox already exists with that name so they know how to fix it.
-
It is confusing for me when I switch to sandbox and see "Live" -> Switch to draft. I know what it means because of the extra context from meetings. But it's a bit strange to come from somewhere where I can't edit a live workflow and then it is marked as "Live" in the sandbox.
Some general comments outside of the scope of this PR of things I noticed for this epic while I'm here:
-
Switch to draft modal - should Esc close this modal?
-
Switch to draft modal: I'm not sure the message is clear enough that it will turn off the production workflow. Not sure of the wording but perhaps needs to warn more explicitly.
-
At one point I forgot I was reviewing this PR, went to rename my workflow and got confused why I couldn't edit anything. Perhaps this tooltip should be extended to mention why you don't have permission to edit and how to edit? (however, you might not be able to mention sandboxes as the experience is different for free vs paid users)
- The box shadows of the new buttons feel very different stylistically compared to the other parts of the application. Are they part of the new design system?
-
You cannot copy or see the full webhook URL for a live workflow
-
Go live button is the wrong colour when disabled (should be the same as Save)
-
I see the old trigger picker.. you should merge from main to avoid any nasty conflicts while you can 🤭
-
The top bar feels very hectic.. there is a LOT of things going on up there now. I'm sure this will get better as other epics come in but it feels like a lot.
| } | ||
| end | ||
|
|
||
| defp collaborator_name(user) do |
There was a problem hiding this comment.
Non-blocking. Claude mentioned how this function is duplicating the first/last name logic that already appears in mailer.ex, workflow_json.ex and elsewhere. There isn't a canonical helper yet. But we could extract this out to a shared Accounts.display_name/1 function (not sure if that's the right place) and replace all the copies. This is out of scope of this PR but flagging for follow-up.
There was a problem hiding this comment.
Agreed it's worth doing, but it touches mailer.ex, workflow_json.ex and a few others that have nothing to do with this PR, so I've left it as a follow-up: #4988.
Picker: - List only joinable sandboxes, and hide the join section when there are none - Show each row with the creator's avatar, name and the sandbox's creation time - Require a name to create a sandbox - Truncate long workflow names in the header so the save action stays visible Backend and channel: - Consolidate provisioning and promotion with a compensating delete so a failed promote leaves no orphan sandbox - Gate content edits on a live workflow server-side; keep lifecycle transitions role-only - Handle the nesting-too-deep error and add a catch-all so the channel cannot crash the socket - Restrict the sandbox listing to users who can create or join - Extract a shared workflow lookup and colour helper, drop duplicated code Editor: - Surface field-level channel errors through the notifications service instead of raw toasts
Copying the trigger URL is a read action, but the read-only lock was disabling it along with the real editing controls. Expose the read-only reason from useWorkflowReadOnly and keep copy enabled when the workflow is merely locked (live, or no edit permission), while still disabling it when the shown URL is not a current endpoint: a deleted workflow, a pinned historical version, or an unsaved new workflow. Closes #4990
State that switching to draft takes the workflow out of production and disables its triggers, instead of the vague 'stops processing data'. The dialog already closes on Escape. Closes #4989
Go live, Switch to draft, and Edit in sandbox used shadow-sm, heavier than the shared Button component's shadow-xs, so they stood out from the rest of the app. Align them to shadow-xs. Closes #4991
Review fixes: - Gate "Edit in sandbox" on a new can_provision_sandbox permission, so users who cannot provision see it disabled with a reason instead of an error on click (brings #4920 forward) - Add a "live" read-only reason so live workflows stop showing "you do not have permission to edit", and keep the webhook Copy URL enabled on them - Return only joinable sandboxes from list_sandboxes, server-side - Log the compensating-delete failure in provision_editing_sandbox - Rename the sandbox row's "creator" to "owner" (accurate after a transfer) - Extract a shared channel-error helper in the picker; add nesting_too_deep to the channel error type unions In-sandbox header: - Remove the sandbox badge and hide the draft/live badge inside a sandbox - Replace Go live / Switch to draft with a disabled "Promote" button (coming soon) inside a sandbox
Flatten the nested cards into a single panel separated by a hairline divider; anchor each row with a thin colour stripe from the sandbox's own colour instead of repeated owner initials; make the whole row the click target with a quiet Join that fills in and reveals an arrow on hover; add a loading skeleton, an accessible input label, and relative timestamps with the exact date on hover via the app's Tooltip; refine the create button and drop the beaker icon; replace the bottom Cancel button with a corner close (X).
- Enforce the live-workflow lock on update_trigger_auth_methods (was role-only) - Show the "live" read-only message only to users who can act on it (switch to draft / edit in sandbox); viewers get the plain no-permission message - Enable webhook Copy URL on pinned versions (the endpoint is stable) - Give the truncated header workflow name a tooltip with the full name - Sort the picker list by created time to match the "Created" label, preload only the sandbox owner, and document the intentional save-path re-read - Remove the dead client-side joinable filter and cap the join list height with an internal scroll for users with many sandboxes
|
Thanks for the super thorough review @lmac-1, this was really helpful. I've replied to the code comments inline. On the rest, I've restated each of your points so they're easy to follow: Multi-paragraph comments: collapsed all four blocks to one-liners. UX/UI
Out-of-scope epic notes
Ready for another look when you get a chance. The follow-ups I've split out are #4988 (display-name helper) and #4992 (picker wording). Could you take another look? |
Both were Headless UI dialogs relying on default Escape handling, which the editor's higher-priority IDE/inspector Escape handlers intercept first. Register a MODAL-priority Escape handler on each (matching ConfigureAdaptorModal) so they close on Esc.
Header now calls usePermissions directly for can_provision_sandbox, but this test's useSessionContext mock did not export it, so Header threw on render.
|
Hey @elias-ba, I am doing a more detailed review, but I came across this bug while testing. Not sure if it's on your radar already: I suspect things might sort themselves out when added in with the new 'create' workflow. Maybe. Who knows. |
There was a problem hiding this comment.
Hey Elias, great work on cleaning up all the feedback from before. I really love the way you organise your epics and raise issues for smaller things so they don't get lost.
Looking great except for this part (shouldn't block merge though, would just be nice to clean it up a bit):
Better toast for a duplicate sandbox name: Done, a duplicate name now surfaces the real "has already been taken" error instead of the generic toast.
I see "- has already been taken" rather than "Name has already been taken" or "{sandboxname} has already been taken". This matches a lot of current error messages in the app so it's not terrible, but if it's an easy win, we should clean it up to make it nicer. This is because there might be some sandbox with a name that we don't know about (it doesn't appear in 'active sandbox' list) so the user might be confused to know what has already been taken.
While we're in there, the same error surface has a related problem. If I type My_Sandbox I get "- has invalid format", and if I type something like !!! I get "- can't be blank" for a name I definitely did type. That's because url_safe_name keeps underscores, dots and accented letters, but the project name validation only allows lowercase letters, numbers and hyphens — so the two disagree about what's valid. Same root cause as the message above, so possibly one fix for both. (I think this mismatch predates the PR — the sandbox creation form has it too — but the picker shows it with less context, since there's no derived-name preview to explain what happened.). Not blocking in this PR but flagging as a general UX clean up for later.
I found the following bugs. I know that the scope of this PR might not include all of these, so I will leave it to you to decide what should be covered as part of this PR, and what should be a follow-up:
-
The flow breaks in this scenario:
- In draft status, I edit the name of the workflow
- I click "Go live"
- I click "Edit in sandbox"
- I try to create a sandbox and I get an error "An internal error has occurred" without much information
-
Another bug:
- I have a workflow called "Workflow A" that has webhook trigger and common job.
- I click "Edit in sandbox" from workflow A in a sandbox called "Sandbox A"
- I delete the original workflow A at the project level
- I create another workflow called "Workflow A" that has a different structure, let's say cron job and chatgpt job. Let's refer to this as Workflow B
- I click "Edit in sandbox"
- The sandbox I created in the previous step (Sandbox A) appears as an active sandbox
- I click on that sandbox and it shows me the original Workflow A in that sandbox rather than the new workflow we created with the chatgpt job.
-
Renaming the workflow doesn't seem to update the "active sandboxes" list - is the search looking up workflows inside the sandbox by name?
- Precondition: I already have a sandbox with some workflows from the default event-based workflow template with name "Event-based workflow"
- I rename my workflow from "Event-based workflow" to something else
- I click Go live and then Edit in sandbox
- The active sandboxes shows all the sandboxed with "Event-based workflow" inside rather than this new workflow I have renamed.
- If I refresh the page and click "Edit in sandbox" I correctly get no active sandboxes
-
Escaping out of sandbox creation still takes me into the sandbox:
- I click "Create sandbox"
- While it's creating, I press Escape (or click the ✕)
- The modal closes, but the request is still in flight, and when it succeeds I get navigated into the new sandbox anyway
- The other confirm dialogs in t he editor block closing while a request is in flight, so it'd be consistent to do the same here
Claude says:
I think bugs 1, 2 and 3 are all the same root cause, and it might be worth fixing at that level rather than three times.
The picker works out "which sandboxes contain a copy of this workflow" by matching on workflow name. That's why renaming breaks it (1 and 3), and why deleting and recreating a workflow with the same name matches an unrelated clone (2).
But there's already a lineage link we could use instead: when a sandbox is provisioned,
copy_workflow_version_history/2copies the parent workflow's latestworkflow_versionshash onto the clone, so the two share a version hash. The schema docs note that the same hash existing across workflows is expected, and the rows are append-only, so that shared ancestor survives even after the sandbox diverges. Matching on shared lineage rather than name would fix all three at once — and 1 would disappear entirely, since the name would stop being involved.One caveat: two genuinely unrelated workflows with identical content would share a hash, so if that's a concern the cleanest version is probably an explicit "cloned from" column on the sandbox workflow. Your call which is proportionate — but either way I don't think name should be the identity here.
Please advise whether these bugs should be handled in this PR or follow-up work.
A question rather than a change request, I don't know too much about Kafka — but I'd like to understand it. Creating the sandbox makes the cloned workflow live, which switches its trigger on. That makes sense to me for webhook and cron: the sandbox gets a URL nobody knows yet, or runs its own copy on a schedule. Either way it only affects itself, which is what we want so you can see it working.
Kafka feels different though, because the sandbox reaches out and attaches to the same production broker and topics. Those connection settings (hosts, topics, sasl, username/password) live on the trigger config rather than in the credential system, so unlike normal credentials they aren't scoped by the sandbox's dev environment. The clone gets a fresh group_id, so production still receives everything — but every message would get processed twice, once by production and once by the sandbox.
Is that OK for how we use Kafka, or should Kafka triggers stay switched off when the sandbox is created?
2 participants
Description
This PR adds "Edit in sandbox". From a live workflow, the header gains an "Edit in sandbox" action that opens a picker to either branch a new sandbox from the current live version or join an active one, then lands the user in the sandbox editor with that workflow's trigger live and its own endpoint. The parent project is untouched; changes come back through promotion in a later slice of the epic.
Creating clones the parent project (the existing full-clone provisioning), then promotes the edited workflow to live inside the sandbox. The other cloned workflows stay draft, so a workflow's state stays coherent with its triggers. The sandbox copy remains editable because the live read-only lock only applies outside a sandbox. The picker lists only the parent's active sandboxes that already contain this workflow (last edited first), each showing the sandbox's creator and creation time; a name is required to create a new one. A sandbox badge is shown in the editor header. Server-side errors (permission, usage limit) surface their real message in the picker, including the limit upsell, rather than a generic toast.
Closes #4859
Closes #4989
Closes #4990
Closes #4991
Validation steps
Additional notes for the reviewer
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just want to know!):
You can read more details in our Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code):owner,:admin,:editor,:viewer)