feat(experiments): change the page of a draft that has variants - #37306
Merged
Conversation
The Page card offered Clear (an X with a tooltip) once a page was chosen, and disabled Select outright on a draft that already had variants — the server refuses the change while a non-control variant holds a copy of the page. Now the card offers Change Page, and the variants are a step on the way rather than a wall: - No non-control variant, or no experiment yet: straight to the picker. - Otherwise a confirmation names the page and every variant the change would delete, and deletes them behind a red Delete Variants And Change Page. Only when they are all gone does the picker open. The deletions run one after another (concatMap): each has the backend recompute the traffic proportion over what is left, so only the answer to the last one describes the experiment afterwards. A rejection halfway keeps the answer the run did get, so the Variants card stops offering weights for rows that no longer exist, and is reported unobtrusively — a toast, not an alert over the open dialog, which would bury the retry. The confirmation is a view: it renders what it is handed and reports the go-ahead. The wait and the failure reach it as store signals through inputValues (applied once via setInput, hence signals rather than values), and the Page card — which owns both the store and the dialog reference — is what closes it on deleteVariantsSucceeded. Both dialogs go through DialogService, so the handover stays one onClose chain. Gone with the X: the pageCleared event, and the two tooltip keys that were unreachable once a non-draft experiment reads with the same locked copy as every other field. VARIANT_COLORS moved to shared/constants, since the dialog draws the same variants in the same colours as the card.
10 tasks
Contributor
|
Claude finished @oidacra's task in 3m 41s —— View job Code ReviewReviewed the full diff against New Issues
Resolved
Everything else (concatMap ordering, Note on the flagged spec divergence (#37176 FR-009): that's a product/ownership call, not a code defect — correctly escalated in the PR body rather than resolved unilaterally. · branch |
github-actions
Bot
added
the
Area : Frontend
Follow-up on the review of this branch. Five findings, all on the destructive path: The confirmation could be dismissed with X or ESC while the deletions were still running. Only the Cancel button was gated on the in-flight state; PrimeNG's own chrome calls close() directly. The run does not stop with the dialog, so a half-finished cascade could finish with nothing on screen reporting it, and the card was left holding a reference it could no longer close — which made the eventual success a no-op, so the picker never opened. Both halves are fixed: `closable`/`closeOnEscape` are now getters over `deletingVariants` (PrimeNG re-reads them on every change detection), and the success handler opens the picker whether or not a dialog is left to close. The dialog's variant list was a snapshot taken at open time, while `deleting` and `failed` were signals — for exactly the reason the list needed to be one too. After a run refused halfway it went on naming, and offering to delete, variants that were already gone. It is a signal now. The failure copy said the Variants could not be deleted "so the Page has not changed", which reads as nothing happened when some may be permanently gone. It now says which ones are left. The store had no re-entrancy guard: the dialog guarded its own button, but the store took every dispatch, and `switchMap` would cancel a run whose deletions had partly landed to start another from a list that had not caught up. `deletingVariants` now closes the door, raised by a new `deleteVariantsRequested` the handler emits as the first call leaves — the contract `create$` and `start$` already keep, and the only one that works, since a reducer on the intent runs before the handler and its guard would refuse the first press. A confirmation with nothing left to delete is answered as success rather than dropped. It is reachable — the variants can go from another tab between opening the dialog and confirming it — and the page is free either way, so dropping it left the dialog waiting on a run that never went out. Also corrects the handler's comment, which claimed the event "cannot arrive" without variants while the guard and a test right beside it exist because it can. 931 tests.
…orm does No icon. The warning triangle led the dialog with decoration where the other confirmations on the platform lead with the sentence, so it is gone and the paragraph is the first thing read. The header asks the question — "Change Page?" — and the paragraph answers it: what changing the page costs, and that it cannot be undone, in one block instead of a grey line under the list. It names the page by title, in quotes, rather than by path. Neither button says nothing any more: "Keep Current Page" against "Delete Variants And Change Page", so both choices are legible without reading the paragraph again. Still not a `p-confirmDialog`, which is the usual vehicle for this: that one closes on accept and has nowhere to put a wait or a failure, and this operation has both — the button spins while the deletions run and the dialog stays open, with the failure inline, for the retry. Copy note: the mock this follows reads "If you delete <page> all its Variants will be deleted" with a "Delete Page and Variants" button. Both say the page is deleted, which it is not — it is repointed — so the shape is adopted and the wording corrected. 933 tests.
10 tasks
…ge-of-a-draft-with-variants
Ports the migration done in the results-screen worktree onto this branch, where the Page card now lives. `openPageSelector()` opened `DotBrowserSelectorComponent` with a hand-built dialog config. It now goes through `AngularAssetPickerLauncher`, in browse mode narrowed to pages by `allowedBaseTypes`, with the browse flags the picker actually understands. The launcher is stateless and carries no `providedIn`, so it is provided beside `DialogService`. The site is no longer an id: the picker browses a `DotSite` and, unlike the browser it replaces, has no System Host to fall back on. With none resolved the press is a no-op rather than a dialog opened against nothing. `SELECT_PAGE_BROWSER_PARAMS` loses `showPages`/`showFiles`/`showDotAssets` — what the list may contain is `allowedBaseTypes`' job now — and the two dialog-width constants go with the config that used them. `showWorking` stays: omitting it would read as the same thing but `false` is the only value that narrows, and the old screen listed drafts. Both entry points still funnel through `openPageSelector()`, so the Change Page confirmation reaches the new picker exactly as the happy path does. Requires the merge of main in the preceding commit: `allowedBaseTypes` and `browse` do not exist on this branch's fork point. 934 tests.
…ft-with-variants' into issue-37003-change-page-of-a-draft-with-variants
The site was checked where the picker opens. Between the Change Page press and that point sits the deletion of every non-control variant, so with no site resolved the flow ran the whole irreversible cascade and then found nothing to open the picker against: the user paid the entire cost and got none of the screen they asked for. Checked at the press instead, where nothing has been spent yet. The no-site press stays the same bare no-op it already was on the direct path — the decision not to disable the button and explain itself is untouched; it just now applies before the destructive step rather than after it. 935 tests.
It was `severity="danger"`. The design it follows draws it as the primary button, and so does every other confirmation on the platform — the repo's own way of marking the primary action is to omit `severity` altogether, as the Add Variant dialog's submit does. The weight is carried by the labels, which say what each choice does: "Keep Current Page" against "Delete Variants And Change Page".
adrianjm-dotCMS
previously approved these changes
Sep 1, 2026
Two of the three review threads. **A dead click becomes a disabled button.** `canChangePage()` requires at least one variant, `$isPageActionDisabled` only asked about the lock, and the early return swallowed the press — so a draft whose variants array is empty offered a button that did nothing and said nothing. The reviewer preferred dropping the `variants.length > 0` clause, on the grounds that a draft with no variants has nothing to orphan. The server disagrees: `hasOnlyTheControlVariant` is `variants.size() == 1 && first().id() == DEFAULT`, so zero variants is refused there too. Dropping the clause would have the card send a PATCH the API rejects. Disabled it is, which keeps both sides saying the same thing. **The picker no longer opens for a run this card did not start.** `deleteVariantsSucceeded` is a global event and the card's answer to it is to open a dialog. Only this flow can produce it today, so `#awaitingPageChange` changes no behaviour — it writes the invariant down instead of leaving it resting on there being one producer. The test that covered "the answer arrives with no confirmation left" now asserts the opposite for a run that was never started, and two more cover a second answer and a cancelled confirmation. The case it used to protect — dismissed mid-run — is unreachable since the dialog withdrew `closable` and `closeOnEscape` while deleting. 938 tests.
6 tasks
oidacra
enabled auto-merge
September 1, 2026 16:25
adrianjm-dotCMS
approved these changes
Sep 1, 2026
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
2 participants
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.
Proposed Changes
Follow-up on the Configure screen (Screen 2) of the Experiments portlet: changing the Page of a draft that already has Variants.
CleanShot.2026-08-31.at.15.56.39.mp4
/experiments/new— goes straight to the page picker, with no confirmation in the way.ExperimentsAPIImpl.save()refuses a page change while a non-control Variant holds a copy of the current page, so the change deletes them first. A dialog names the page and every Variant by name, states that the action cannot be undone, and puts the deletion behind the primary Delete Variants And Change Page, against Keep Current Page. Only when they are all gone does the picker open; Cancel, the X and ESC leave the experiment exactly as it was — and none of the three is available while a deletion is on the wire, since the run does not stop with the dialog.concatMap). Each has the backend recompute the traffic proportion over what is left, so only the answer to the last call describes the experiment afterwards — firing them in parallel would leave which answer is current up to arrival order.Removed along with the X: the
pageClearedevent, and the two tooltip keys that became unreachable once a non-draft experiment reads with the generic locked copy.VARIANT_COLORSmoved toshared/constants.ts, since the dialog draws the same Variants in the same colours as the card.Checklist
Additional Info
How the pieces are split. The confirmation is a view: it renders what it is handed and reports the go-ahead as
pageChangeConfirmed. The wait and the failure reach it as store signals through PrimeNG'sinputValues— signals rather than values, becauseinputValuesis applied once viasetInputwhen the dialog is created, so a boolean would freeze at whatever it was on open. The Page card owns both the store and theDynamicDialogRef, so it is what closes the dialog ondeleteVariantsSucceeded; the dialog is created outside the card's injector and cannot reach the store at all. Both dialogs go throughDialogService, so the handover stays oneonClosechain.deleteVariantsSucceeded/deleteVariantsFailedare deliberately their own pair rather than a pluralremoveVariant*: the common case deletes exactly one Variant, so the payload cannot tell the two flows apart, and they differ in error surfacing (toast vs. the standard alert) and in state. Same precedent asabortSucceededvscancelScheduleSucceededin this store — one call, two events, because the consequence differs.Two behaviours worth exercising by hand. The deletion is irreversible and happens before the picker: confirming and then cancelling the picker leaves the Variants deleted and the page unchanged. And the new page is persisted by Save Draft, not by picking it —
pageIdtravels in the footer's PATCH, as it already did.Verification. 935/935 portlet tests,
nx lintclean,nx format:checkclean,nx affected -t testgreen across the 4 affected projects.specs/37176-draft-experiment-page-change/spec.md— committed onmainand reviewed twice (7a280d6698,54d378bcb8) — specifies the opposite affordance for this exact situation:experiments.configure.page.select.has-variants.tooltip).This PR ships cascade-delete-then-proceed instead of disable-and-explain. It is a deliberate product decision, not an oversight, and the backend rule is untouched:
ExperimentsAPIImpl.save()still requires DRAFT + control-is-the-only-variant (1fbdeb837d), which is why the variants are deleted first rather than the rule being relaxed.What that means for this review:
specs/37176-draft-experiment-page-change/is updated by this PR, so the committed spec now contradicts what ships. Read on its own, it will tell the next person the picker is disabled.CLAUDE.md("If the spec changes after sign-off, get it re-approved"), the call belongs to whoever owns that spec: either FR-008/009/010 + User Story 3 get rewritten around the new flow and re-approved, or this flow goes back to disable-and-explain.I left the spec untouched on purpose rather than rewriting a signed-off document unilaterally. Flagging it here so it is a decision rather than a discovery.
Review of this branch
A multi-agent review of the first commit surfaced five defects on the destructive path, all fixed in
42b120f581:close()directly. The run does not stop with the dialog, so a half-finished cascade could complete with nothing reporting it, and the card kept a reference it could no longer close — making the eventual success a no-op, so the picker never opened.closable/closeOnEscapeare now getters overdeletingVariants, and the success handler opens the picker whether or not a dialog is left to close.deleting/failedwere signals — for exactly the reason the list needed to be one. After a partial failure it went on naming, and offering to delete, variants that were already gone.switchMapwould cancel a run whose deletions had partly landed.deletingVariantsnow closes the door, raised bydeleteVariantsRequestedas the first call leaves — the same contractcreate$/start$keep.Not changed, and worth a second opinion:
canChangePage()requiresvariants.length > 0, so a draft with zero variants reads as immutable. Unreachable today (the creation POST always makes the control), and it mirrors a server-side rule, so I left it alone rather than widening the change.Refs #37003
This PR fixes: #37003