Stop sandbox merges from deleting the parent project's collections - #5054
Open
elias-ba wants to merge 7 commits into
Open
Stop sandbox merges from deleting the parent project's collections#5054elias-ba wants to merge 7 commits into
elias-ba wants to merge 7 commits into
Conversation
Security Review ✅
|
elias-ba
changed the title
elias-ba
force-pushed
the
fix/sandbox-merge-collection-deletions
branch
from
August 8, 2026 00:15
3b754ec to
3d52c5e
Compare
Merging a sandbox synchronises the target's collection names with the sandbox's, which meant collections that existed only on the target were silently hard-deleted with all their items when an owner or admin merged. A user lost their parent project's collections this way without the merge screen ever mentioning them. Deleting target-only collections is now opt-in: the merge keeps them unless the caller passes delete_collections: true, and even then only when the actor has owner/admin collection rights on the target. The merge modal now lists the collections a merge would create in the target and, separately, the ones that exist only on the target, with an unchecked-by-default checkbox to delete them (shown only to users who may). A new preview function exposes the same diff the sync uses so the UI cannot drift from what the merge actually does.
Review follow-ups on the opt-in behaviour: - The delete checkbox no longer loses its state when any other checkbox in the merge form is toggled: the form's change event now only recomputes the collections preview (and resets the opt-in) when the selected target actually changed. This also stops re-running the preview queries on every form event. The checkbox itself now follows the modal's select-all pattern (click handler on the input) instead of a label-level handler with a readonly input, which double-fired and netted to a no-op when the label text was clicked. - The merge now deletes exactly what the user was shown: the modal passes the previewed collection ids into the merge, and the sync deletes only collections that are both still target-only and in that list. A collection added or renamed in the target while the modal was open is kept. - The opt-in is only honored for the target it was previewed against, and the permission is re-checked for the submitted target when the merge is confirmed. - Deduplicated the modal-state resets for the collection assigns.
The two collection tables in the merge modal are now a single Collections panel with one row per affected collection (unchanged collections are not listed). Each row carries a status badge: sandbox-only collections show a green "Will be added", target-only collections show a neutral "Kept" that flips to a red "Will be deleted" in place when the delete opt-in is checked. Target-only rows also show their item count and data size, fetched with a single grouped count query, so the user can weigh the opt-in. The panel footer holds the opt-in checkbox for owners/admins - copy now names the item count it would remove - or, for everyone else, a note that target-only collections are always kept. Behaviour is unchanged: deletion still requires the opt-in, stays pinned to the previewed collections and target, and resets when the target changes.
Reworks the merge modal's collections UI to match the other panels instead of using its own pattern: - "Collections to add" mirrors the credentials panel: one checkbox per sandbox-only collection, all preselected, with select-all in the header. Unchecked collections are simply not created in the target. - "Collections to delete from <target>" uses the same structure with red accents, one checkbox per target-only collection, none selected by default, select-all, and the item count and data size per row. It only renders for users who may delete collections on the target; everyone else sees the same rows without checkboxes and a note that target-only collections are always kept. The merge now carries both halves of the choice: the names to create and the ids to delete, each pinned to what the modal showed for the submitted target. Defaults with no options stay as before - create all source-only collections, delete nothing - so other callers keep their behaviour. All the earlier safeguards remain: selections survive form change events for an unchanged target, reset when the target changes, deletion re-checks the permission at confirm time, and a crafted event for a different target falls back to the safe defaults.
Collections are runtime data stores, so removing one is a project settings action, not something a merge should offer. This strips the deletion half of the merge's collection handling: - The merge modal keeps only the "Collections to add" panel (per-collection checkboxes, preselected, select-all). Collections that exist only in the target are not mentioned; the merge simply leaves them alone. - The merge no longer takes any deletion option. Deletion-shaped options from stale or crafted callers are ignored, and the collection sync itself has no delete branch left: it only creates the source's missing collections (optionally narrowed to the previewed names). - Tests now pin the invariant directly: target-only collections survive owner, admin and editor merges, crafted deletion options, crafted targets, and mid-modal additions. Creation selection behaviour is unchanged and still covered.
Review follow-ups on how the merge decides which collections to create. The modal used to snapshot the preview when it opened and submit that snapshot as the full create list, which quietly excluded any collection added to the sandbox while the modal was open - and since the sandbox is retired after a merge, that collection's data was lost for good. The merge option is now the inverse: the modal submits only the names the user explicitly unchecked, and the merge recomputes what the target is missing at merge time and creates everything except that skip list. New-since-preview collections are created, which is the safe direction. The unchecked names are still only honored for the target they were previewed against; for any other target nothing is skipped, which now lines up with the documented behaviour instead of contradicting it. A malformed skip list (not a list, or non-name members) raises instead of being silently treated as "skip nothing". Also cleans up leftovers from earlier revisions: stale comments about deletion on the collection assigns, an unused actor argument in the confirm path, a transaction wrapper around what is now a single insert, and dead empty-state branches in a panel that only renders non-empty.
elias-ba
force-pushed
the
fix/sandbox-merge-collection-deletions
branch
from
August 8, 2026 18:30
3d52c5e to
952e662
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5054 +/- ##
=======================================
- Coverage 90.7% 90.7% -0.0%
=======================================
Files 420 420
Lines 19952 19981 +29
=======================================
+ Hits 18091 18115 +24
- Misses 1861 1866 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #5055.
This PR fixes an unexpected data loss when merging a sandbox. A user merged a sandbox back into its parent and lost collections on the parent project: the merge quietly made the target's collection names match the sandbox's, so anything the sandbox didn't have was deleted from the parent, items and all, and the merge screen never mentioned collections.
A merge now never deletes collections. Collections that exist only in the sandbox are created empty in the target, and the merge screen lists them in a "Collections to add" section that works exactly like the workflows and credentials ones: a checkbox per row, all preselected, with select-all — unchecked ones are simply not created. Collections that exist only in the target are always kept and the merge doesn't mention them; deleting a collection remains available in the project's settings, where it belongs.
Validation steps
Additional notes for the reviewer
Removing the deletion path is deliberate: a merge can never delete a collection, whatever role or options the caller has.
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)
(e.g.,
:owner,:admin,:editor,:viewer)