feat(experiments): flagged UVE entry point and variant Edit Content round-trip (#37005) - #37256
Open
oidacra wants to merge 32 commits into
Open
feat(experiments): flagged UVE entry point and variant Edit Content round-trip (#37005)#37256oidacra wants to merge 32 commits into
oidacra wants to merge 32 commits into
Conversation
8 tasks
Contributor
|
Claude finished @oidacra's task in 5m 0s —— View job Code Review — PR #37256 (flagged UVE entry point & variant round-trip)Reviewed the implementation diff (frontend entry-point switch, deep-link builders, list/configure stores, backend flag). The code is unusually well-documented and the behavior matches the spec's FRs closely. One concurrency observation below; the rest is clean. New Issues
Notes (non-blocking, no action required)
Test coverage for the new units (deep-link builder, switch reader, breadcrumb util, page filter, read-only rules) is present and substantial. Note: the PR branch is behind · branch |
oidacra
requested review from
adrianjm-dotCMS,
erickgonzalez,
hmoreras,
jcastro-dotcms and
nicobytes
August 27, 2026 18:22
erickgonzalez
previously requested changes
Aug 28, 2026
erickgonzalez
left a comment
erickgonzalez
left a comment
Member
There was a problem hiding this comment.
Feedback:
- Missing user story to turn off the FF in case something goes wrong
freddyDOTCMS
previously approved these changes
Aug 28, 2026
…37005) Specification for the Experiments Portlet UVE integration: the flagged entry-point switch plus the variant Edit Content round-trip. Three points in the issue did not survive contact with the code, and the spec resolves each one (recorded as D1/D2/D3 with rationale, since a reader would otherwise assume the issue text is correct): D1 - The issue proposes reviving FEATURE_FLAG_EXPERIMENTS, calling it "declared, zero consumers today". That holds for the frontend enum entry only. The property is the live backend kill-switch for the whole Experiments feature (ConfigExperimentUtil.isExperimentEnabled, read by ExperimentWebAPIImpl and HTMLPageAssetRenderedAPIImpl), so setting it to false - the issue's intended default - would stop running experiments from serving to visitors. The spec requires a new, dedicated switch, shipped explicitly off, and leaves the kill-switch untouched. "Off by default" also needs an explicit shipped default, because getFeatureFlags and withFlags both map an absent flag to enabled. D2 - The issue defers the flag-on destination. The spec settles it: the site-wide list filtered to the page in hand, with defined behavior for zero, one and many experiments, and a filter the editor can clear. D3 - The issue asks a variant's meta line to read "Edited in the Universal Visual Editor". No such signal exists on the variant model on either side of the wire, and the legacy per-page card displays no edit indication at all, so the spec forbids inventing one. It does require one fix that follows from that: the new Variants card renders the fixed string "No content changes yet" for every non-control variant, true today only because the Edit Content button beside it is disabled. This work is what makes it capable of being false, so the claim has to go. Also records an impact on #37008, which currently says to remove FEATURE_FLAG_EXPERIMENTS during migration. Under D1 what gets removed is the new switch; the property must survive. Spec only - no implementation.
oidacra
force-pushed
the
issue-37005-experiments-uve-integration
branch
from
September 2, 2026 14:47
d88816a to
21bd1ef
Compare
…ted flag (#37005) Phase 1 design artifacts for the UVE integration spec. plan.md, research.md and quickstart.md stay local per the repo's Spec-Kit convention (.gitignore:227-232); data-model.md and contracts/ carry the verified contracts and are tracked. - contracts/entry-point-switch.md: FEATURE_FLAG_EXPERIMENTS_PORTLET — name, the three additive backend edits, wire format, read path and failure behavior. The explicit `false` in dotmarketing-config.properties is required, not decorative: every flag reader in the repo maps an unset property to enabled, and omitting the key from ConfigurationResource's WHITE_LIST fails open and silently. - contracts/navigation-destinations.md: the URL table the FR-016..FR-019 regression tests assert against — the nav item, the variant deep link, the return leg, and the running-experiment tag, per switch branch. - data-model.md: no persisted entity changes. Two client-side view-model additions: DotExperimentConfigurePage.languageId (UVE's guard substitutes language_id=1 when absent, which FR-004 forbids) and DotExperimentsListState.selectedPageId (matched by identifier equality, never a path substring). Two findings for review, both recorded in the plan: - The running-experiment tag is not switched: the new portlet has no :id/results route pending #37004, so switching it would route into a 404. FR-017 constrains only the switch-off case and stays satisfied. - FR-007's premise is stale. The variant-meta line it describes does not exist on main — #37003's implementation redesigned the Variants card into a table with no per-variant state text — so the requirement is already satisfied and becomes a regression assertion. FR-007a and SC-008 unaffected; the spec is not amended here. ADR gate: no ADR in dotCMS/platform-adrs governs feature flags, frontend routing, UVE navigation or legacy/new UI coexistence. ADR-0013 applies as CI scope only (the properties edit makes this PR not frontend-only, so the merge queue runs the full Integration and Postman sets). No conflicts with any accepted ADR.
…he switch (#37005) Writing the task list exposed a conflict in the return-leg contract: branching the variant chip's destination on FEATURE_FLAG_EXPERIMENTS_PORTLET alone makes FR-018 and FR-005/FR-027 mutually exclusive on a supported path. Switch off (the shipped default), the editor reaches the portlet from the main navigation — FR-026 keeps it reachable at any switch value — opens a variant with Edit Content, which FR-027 keeps working at any switch value, and returns via the chip. A switch-only branch lands them on the legacy UVE-embedded screen they never came from, violating FR-005 and FR-027 outright. Honouring FR-005 unconditionally instead would break FR-018. The switch is the wrong question at this point: FR-027 says in as many words that it governs how an editor *reaches* the portlet, not where a round-trip that already started there comes back to. The destination is now resolved by origin, with the switch as the fallback: - portlet origin (marker set by the outbound leg) -> /experiments/{id}/configuration - legacy origin (sets no marker) -> /edit-page/experiments/{pageId}/{id}/configuration - no marker (pasted or bookmarked variant URL) -> whichever the switch selects This is what the spec's own deep-link edge case already implies — "must *still* land somewhere coherent for the switch's current value" only means something if the switch is the fallback, which presupposes a primary resolver. The legacy row is the existing code path, untouched, so FR-018's "as before" holds by construction rather than by re-derivation. The marker is a query param, never storage: it has to survive a reload and a pasted link, and be absent rather than stale on a deep link. FR-006 extends to clearing it alongside mode, variantName and experimentId.
…d D3 (#37005) FR-007 and D3 were written on 2026-08-26 against an earlier iteration of the new Variants card. #37003's implementation PR (#37064, merged 03a5456 on 2026-08-31) redesigned that card into a table — Variant, Split, Of Page Traffic, Actions — and the per-variant meta line the spec describes did not survive it. There is no data-testid="variant-meta" element on main and no "Unmodified Page content" or "No content changes yet" string anywhere in core-web or Language.properties. The requirement is unaffected; only its premise was stale. FR-007 keeps its meaning as a regression assertion — the card MUST NOT assert whether a variant's content has been modified — but now reads as a constraint to hold rather than a change to make, because the shipped card already makes no such claim. The card's own description ("Content for each Variant is edited in the Universal Visual Editor") says where editing happens and asserts nothing about any particular variant, so it stays. Named the string this work actually removes: the disabled Edit Content button's tooltip experiments.configure.variants.edit-content.unavailable ("Editing Variant content is available in an upcoming release"), which stops being true when FR-001 lands and comes out with the placeholder it explains. Every FR id is unchanged. FR-007a and SC-008 are untouched — the prohibition on inventing an edited signal and the criterion that no screen tells an editor whether a variant changed were already correct. Sections edited: "The one thing D3 does leave to fix" (now "What D3 leaves to guard"), FR-007, the #37003 dependency note, and D3's "What this still requires". Nothing else in the spec changed.
Member
Author
|
Spec updated after approval, re-review requested. Commit 2085fc4 rewords FR-007 and D3 (plus the #37003 dependency line and the D3 rationale in Section D3). Reason: the spec was written against a Variants card that rendered a fixed per-variant meta line ("No content changes yet"). The implementation of #37003 (PR #37064, merged Aug 31) shipped the card as a table with no per-variant state text, so that premise no longer holds. FR-007 keeps the same requirement (the card must not assert whether a variant's content was modified) but now reads as a constraint to hold rather than a change to make, and it names the string this work actually removes: the disabled Edit Content tooltip Also recorded: the open PM/PO question on D2 (switch-on destination is the site-wide list filtered to the page) was resolved as specified, so FR-021a-c, US3 and SC-009 stand. Planning artifacts added since approval: data-model.md and contracts/ (commits 5624aed and 37e8ba6). plan.md, research.md and tasks.md are gitignored per repo convention. @freddyDOTCMS could you re-approve the spec? @erickgonzalez FYI. |
…ariant deep link (#37005) Red commit for Phase 2 (Foundational) of the #37005 task list: the entry-point switch end to end, the page model's language, and the variant deep-link builder. No implementation — every test below fails, by design. Backend - ConfigurationResourceTest gains five cases for FEATURE_FLAG_EXPERIMENTS_PORTLET, following the file's existing defined / undefined / not-whitelisted pattern. The load-bearing one is that the key is PRESENT in the response: a key missing from WHITE_LIST is not an error, it is dropped, getKey substitutes NOT_FOUND, and the frontend reads the switch as enabled — FR-013 inverted, with nothing logged. The two value cases would keep passing in that state because they stub the flag's own value; only the membership case fails. - ConfigExperimentUtilTest is new and asserts a negative: the entry-point switch never moves isExperimentEnabled(), which gates whether running experiments reach site visitors (FR-014, FR-015a, SC-003). It also guards a latent coupling the mandated name creates. notify() matches with event.getKey().contains(FEATURE_FLAG_EXPERIMENTS_KEY) — a substring test — and "FEATURE_FLAG_EXPERIMENTS_PORTLET" contains "FEATURE_FLAG_EXPERIMENTS", so a system-table write to the new switch also reaches the kill-switch branch and re-resolves it. Not a live defect: resolveFeatureFlag() re-reads the kill-switch by name and lands on the correct value. Left as a guarded observation rather than a fix, because tightening notify() to an equals match would change kill-switch behaviour, which FR-015a forbids. Frontend - dot-properties.service.spec.ts pins the new key's read contract, including two cases for behaviour this work does NOT want: an absent property, and a key absent from the response, both resolve to TRUE. They are why the explicit FEATURE_FLAG_EXPERIMENTS_PORTLET=false is required rather than decorative. - experiments-portlet-switch.util.spec.ts covers the two properties that are invisible in the UI when wrong: a failed read resolves to false instead of erroring (FR-015), and the uncached reader is used so an operator's flip lands on the next gesture (SC-002). Both assert the negative too — the error is not propagated, and getFeatureFlag is not called. - dot-experiments-configure.util.spec.ts covers languageId through both page constructors, and asserts it stays undefined when the source has none rather than defaulting to 1. editEmaGuard substitutes language_id=1 for a missing param instead of rejecting, so defaulting at any layer turns a refusable action into a silent wrong-language open. - dot-experiments-uve-link.util.spec.ts covers the outbound leg against contracts/navigation-destinations.md section 2: the full URL and every parameter, the page's real language, the origin marker the return leg reads, the literal DotPageMode wire values (not UVE_MODE's), five refusal cases that return null rather than a partial destination, and indifference to window.location — asserted by pushing a misleading URL into history and checking the output is unmoved. Red evidence: backend 5x "cannot find symbol: variable FEATURE_FLAG_EXPERIMENTS_PORTLET"; data-access 5 failed / 13 passed, so the 13 pre-existing cases stay green and the new ones are the only failures; tsc reports TS2307 for both new util modules plus TS2339/TS2353 for languageId and the enum entry.
…p-link builder (#37005) Green half of Phase 2 (Foundational). Turns the tests from b1c0517 green without touching the legacy screens, the experiments kill-switch, or any REST contract. The switch — FEATURE_FLAG_EXPERIMENTS_PORTLET - FeatureFlagName gains the constant, with javadoc scoping it against FEATURE_FLAG_EXPERIMENTS and recording that the properties default is required rather than decorative. - ConfigurationResource adds it to BOTH BOOLEAN_FEATURE_FLAGS and WHITE_LIST. The comment says why both: omitted from WHITE_LIST the key is silently dropped from the response, getKey substitutes NOT_FOUND, and the frontend reads the switch as enabled. - dotmarketing-config.properties ships FEATURE_FLAG_EXPERIMENTS_PORTLET=false beside the other UI switches. - readExperimentsPortletSwitch() reads it with getFreshFeatureFlag (uncached, so an operator's flip lands on the next gesture — SC-002) piped through catchError(() => of(false)) (an unreadable switch behaves as off — FR-015). The catchError sits at the call site, not in DotPropertiesService, whose error behaviour every other flag consumer depends on. The page's language DotExperimentConfigurePage gains a required languageId, copied through by both toConfigurePage and fromBrowserPage. Required rather than optional on purpose: editEmaGuard substitutes language_id=1 for a missing param instead of rejecting, so a page without a language yields a link that silently opens the wrong language's content. Making it required surfaced a second construction site: the Page card's AssetPicker handler built the shape inline and would have dropped the language. It now goes through toConfigurePage, so the picked page and a prefilled or loaded one are narrowed identically — one mapping instead of two that can drift. The deep-link builder dot-experiments-uve-link.util.ts builds the outbound leg from store data only, never from window.location (FR-003), and returns null rather than a partial destination when the page has no path or no language (FR-004). buildPageEditorLink is the same builder without the variant, experiment, mode and origin marker, for the list chip's return affordance (FR-024). Two corrections to the plan, both caught by the tests written for them: - mode sends UVE_MODE, not DotPageMode. research.md R4 had claimed the two differ on the wire and described DotPageMode's values as 'EDIT'/'PREVIEW'. Both halves were wrong: DotPageMode.EDIT is 'EDIT_MODE', identical to UVE_MODE.EDIT, and the enums diverge only at LIVE ('ADMIN_MODE' vs 'LIVE'). UVE_MODE is what UVE declares for the param (DotPageApiParams.mode) and compares against, so it is what the builder sends; the test pins the literal strings so the coincidence cannot become the contract. - The persona id is a named local constant, not an import. DEFAULT_PERSONA lives in the edit-ema portlet's own consts and is not public API of that lib; libs/edit-content already inlines the same string twice for the same reason. Unlike language_id this one would be harmless to omit — the guard fills it with exactly this value — but sending it keeps the link complete on arrival. research.md R4 and contracts/navigation-destinations.md are corrected to match. Verified: 962/962 in portlets-dot-experiments-portlet, 5/5 in the switch helper, 21/21 backend (ConfigurationResourceTest 18, ConfigExperimentUtilTest 3), data-access 18/18, plus portlets-edit-ema-portlet and dotcms-ui unchanged and green. Lint clean. openapi.yaml unchanged, old/ untouched, and ConfigExperimentUtil, ExperimentWebAPIImpl and HTMLPageAssetRenderedAPIImpl all untouched (FR-015a).
github-actions
Bot
added
Area : Backend
…7005) `nx format:check --base=origin/main` — what CI runs — flagged this file. Formatting only; no behaviour change.
oidacra
changed the title
… JSON boolean (#37005) `GET /api/v1/configuration/config` returns the normalised lowercase string `"true"` / `"false"` for `FEATURE_FLAG_EXPERIMENTS_PORTLET`, not a JSON boolean. Confirmed against a running instance: `{"entity": {"FEATURE_FLAG_EXPERIMENTS_PORTLET": "true"}}`. The contract, the plan's test-strategy row and the task description all claimed a native boolean. `BOOLEAN_FEATURE_FLAGS` membership normalises the *value* — `"True"`, `"TRUE"`, `"1"` all arrive as `"true"` — it does not change the JSON type. The mistake was easy to make and worth recording: the set is named `BOOLEAN_FEATURE_FLAGS` and its javadoc opens with "must be serialised as native JSON booleans", then corrects itself two lines later with "The wire format is the normalised lowercase string `"true"` or `"false"` — frontend callers should compare with `=== 'true'`". I took the first line as the contract and propagated it. Nothing was broken by it. `DotPropertiesService.normalizeFlagValue` coerces with `value.toLowerCase() === 'true'`, and both backend tests already asserted the string form — the assertions were right, only their names and the surrounding prose were wrong. The two test methods this work added are renamed from `returnsNativeBoolean*` to `returnsNormalised*String`; the ten pre-existing methods carrying the same misleading suffix are left alone rather than renamed as a side effect. Also corrected: the `BOOLEAN_FEATURE_FLAGS` bullet said omitting the key makes the value "arrive as a string and `=== true` comparisons fail". The real consequence is that the value is passed through unnormalised, so `"True"` or `"1"` reaches the frontend verbatim and its `=== 'true'` comparison reads them as off. 18/18 ConfigurationResourceTest green.
…f splitting it (#37005) With the switch on, the UVE Experiments button did nothing. `navigate()` split every href on `/`, so `'/experiments'` became `['', 'experiments']` — and an empty leading segment does not start with `/`, so the router treated the whole call as a RELATIVE navigation rather than an absolute one. An absolute href is now passed whole: `navigate([item.href], …)`. Relative items are unaffected: `content`, `layout`, `rules/{id}` and `experiments/{pageId}` are still split and still prefixed with `edit-page`, so the switch-off destinations remain byte-identical (FR-016/FR-019). The test was complicit, which is the part worth recording. It asserted expect(commands).toEqual(['', 'experiments']); — the array the implementation happened to produce, rather than where that array sends the user. It passed while the button was dead, and it would have kept passing through any future variant of the same mistake. It now resolves the commands through the real router and asserts the destination: expect(router.serializeUrl(router.createUrlTree(commands, extras))) .toBe('/experiments?pageAsset=page-1'); That also covers the query params, which the array assertion never looked at. Found by manual testing with the flag on — no unit test could have caught it while the assertion described the arguments instead of the outcome. 1439 edit-ema green (66/66 suites), lint and format clean.
…37005) With the switch on, the UVE Experiments item navigated correctly to `/experiments?pageAsset={pageId}` and was then rejected by `MenuGuardService`, which called `goToFirstPortlet()` and ejected the editor out of UVE to `/starter`. The switch was on and unusable, with nothing on screen to explain why. The guard validates the first URL segment against `/api/v1/menu`, and the Experiments portlet is opt-in: it is declared in `portlet.xml` but no UpgradeTask adds it to a layout, so an operator who wants it registers it themselves. Every instance that has not registered it therefore answered the entry point by throwing the editor out. `/analytics` is the precedent and carries no such guard, which is why it resolves on the same instance where `/experiments` bounced — verified against a running instance whose `/api/v1/menu` lists 43 portlets and neither `analytics` nor `experiments` among them. That comparison is what identified the guard rather than the menu as the cause. The route test is inverted rather than deleted: it now asserts the guard's ABSENCE and records why, so re-adding it has to be a deliberate act that breaks a test with the reasoning attached. Verified in a browser, not only in unit tests — which is what the previous two attempts at this bug lacked. Clicking the item now lands on `#/experiments?pageAsset=a9f30020-…`, and the list renders the page chip ("Page: /index · Back to Page · close"), a working back-link to `/edit-page/content?url=%2Findex&language_id=1&com.dotmarketing.persona.id=…`, a clear control, and exactly the one experiment on that page. 2261 dotcms-ui green, lint clean.
…37005) The chip's "Back to Page" link rendered its address with `createUrlTree(...).toString()` and bound it to a plain `href`. That produces the router's internal path, `/edit-page/content?url=...`, which looks correct but is not an address in this app: dotAdmin runs on `withHashLocation()`, so clicking it sent the browser to `<host>/edit-page/content` — outside dotAdmin — instead of `<host>/dotAdmin/#/edit-page/content`. `$pageFilterBackLink` now returns the builder's `commands`/`queryParams` pair and the template binds `routerLink`/`queryParams`, which puts the destination through the active `LocationStrategy` — the only thing that knows about the hash. It stays a real anchor, so middle-click and copy-link still work. The existing test passed with the bug in place: it asserted the `href` contained `/edit-page/content`, which the broken form also satisfied. It now follows the link and asserts where the router landed, so the two forms are distinguishable. Verified red against the unfixed component (1 failure), then green, then in the browser: the click lands on `#/edit-page/content?url=%2Findex&language_id=1` with UVE rendered.
…vice (#37005) The refusal toast added for FR-004 injected PrimeNG's `MessageService`. The UVE shell provides that service on `/edit-page`, but nothing on the `/experiments` route does — so on the new portlet path the variants card threw NG0201 during construction and the whole Configure screen rendered blank. The legacy in-UVE path was unaffected, which is why it went unnoticed. Switched to `DotMessageDisplayService`, which is in the app-wide `providers.ts` and is already what the rest of this portlet uses for toasts, so there is no provider to add anywhere. The card's spec had `MessageService` in its providers, which is exactly what hid the bug: the test supplied what production does not. It is now deliberately absent, so re-introducing the injection fails the suite instead of only the app. Verified in the browser: `/experiments/:id/configuration` renders the Configure body and the variants table; before the fix it was empty with NG0201 in console.
The Experiments screens wrote no breadcrumbs, and nothing else wrote them
either: the trail is built by `processUrl`, which only recognises a URL matching
a main-menu entry. This portlet matches neither test — it is opt-in, so it is
absent from `/api/v1/menu`, and the arrival from UVE carries `?pageAsset=`
without an `mId`, which that matcher rejects outright. So the trail kept
whatever the previous screen left, and since the crumb widget renders the last
crumb as the page title, the Experiments list titled itself with the name of the
page the editor had just left.
The list now appends its own crumb, keeping the page filter in the address so
stepping back lands on the list you were on rather than the site-wide one. The
crumb the UVE shell pushes for the page sits above it and is the way back to the
editor, which is the trail the round-trip wants:
Home > Site > Pages > Home (-> UVE) > [Experiments List]
Returning to the editor from there used to leave "Experiments List" behind.
`processSpecialRoute` truncates when the target URL already appears in the
trail, and the UVE crumb never did: `normalizeQueryParams` drops the default
persona to keep the address short, but `editEmaGuard` treats a missing persona
as an incomplete URL and redirects to complete it — so the URL the router
reported back matched no crumb, and the trail grew instead of rewinding. The
crumb now states the persona explicitly; the address bar is unchanged.
Verified in the browser: on the list the trail reads
`Home > Site > Pages > Home` with "Experiments List" as the title; following the
page crumb back to the editor truncates it to `Home > Site > Pages` with "Home"
as the title. Both new assertions were confirmed red first.
) The Configure screen's Preview action on the CONTROL row opens UVE on the default variant with `experimentReturn=portlet` in the address — but nothing read it. `$infoDisplayProps` returns `null` for the default variant, which is right for an ordinary page (there is no variant to announce), and the variant chip it drives is the only thing that reads the origin marker and offers the return. So that preview landed the editor on a screen with no route home: the chip was absent, and the Experiments item in the nav bar goes to the list, not back to the experiment they came from. The toolbar now falls back to a chip of the same shape when the arrival carries the marker, naming the control ("Viewing Original Variant"). Same `variant` id, so `handleInfoDisplayAction` and the template need no special case and the return resolves by origin exactly as it does for a real variant. Scoped to arrivals carrying the marker on purpose: the legacy in-UVE screens send the same `experimentId` and would otherwise gain a chip they never had, and this work leaves the switch-off path as it was (FR-016, FR-019). A test pins that. Verified in the browser: Preview on the control now shows "Viewing Original Variant", and its back arrow lands on `#/experiments/<id>/configuration` with the screen rendered. Confirmed red first.
Clicking the page's crumb did nothing — or, intermittently, opened a second browser window showing dotAdmin. Both are the same defect. PrimeNG's breadcrumb binds `[attr.target]="item.target"`, so an item created without one renders `target="undefined"` in the DOM. That is not an empty target: it names a browsing context called "undefined", so the browser opens the link there instead of navigating the current window. The shell's page crumb was the only crumb in the app created without a `target` — the menu-built crumbs, the Edit Content host and the experiments list all pass `_self`. It took a while to see because every other signal looked right: the href was correct, the click reached the document with `defaultPrevented` false, and assigning the same URL to `location.href` navigated fine. What gave it away was that the "Pages" crumb worked and this one did not, and the only difference in the rendered markup was `target="_self"` versus `target="undefined"`. Verified in the browser: the crumb now navigates the current window to `#/edit-page/content?url=%2Findex&language_id=1&mode=EDIT_MODE` with UVE rendered, one tab open, and the trail truncated back to `Home > Site > Pages`. Confirmed red first.
10 tasks
`DotAnalyticsErrorComponent` has been on main since #36732 reading three keys that were never added to `Language.properties`: analytics.error.not.available analytics.error.not.available.subtitle analytics.error.retry `DotMessageService.get` falls back to the key itself when it finds no entry, so the panel currently renders the literal string "analytics.error.not.available" as its title, and its button reads "analytics.error.retry", wherever Content Analytics is unreachable. Unrelated to #37005 — the change was sitting uncommitted in this worktree and is committed here rather than dropped, since the keys are what the shipped component expects.
…e top (#37005) Configure is four stacked cards tall and the variant round-trip starts and ends at the third of them, so coming back landed the reader at the top of the form, above the card they had just acted on. The return leg now carries `section=variants`, and Configure scrolls that card into view once the experiment is in. A query param rather than navigation state: `Navigation.extras.state` is only readable while the navigation is in flight, which races the component's own construction, whereas the param is boring, survives a reload and says what it means in the address. Keyed on `loadSucceeded` rather than on a signal, because the cards only exist once the experiment is there — the same shape, and the same reason, as the existing scroll-to-first-validation-error. The anchor lives on the element the screen scrolls (`data-testid="configure-section-variants"`) rather than reaching into the card's own test id. Only the portlet destination sets it; the legacy screen's behaviour is unchanged (FR-018). Entering from the list or creating an experiment still lands at the top, and a test pins that. Two `ActivatedRoute` doubles in the Configure spec were missing `queryParamMap`, which the real thing always has; they now provide it. Verified in the browser end to end: Edit on Variant A, back through the chip, landing on `?section=variants` with the scroll container at 686 (0 on a plain entry) and the card on screen. Both new assertions were confirmed red first.
oidacra
marked this pull request as ready for review
September 4, 2026 16:32
…Save Draft
A page change was only written when Save Draft was pressed, and nothing stopped
the editor from touching the rest of the form in between. That window is not
milliseconds — it lasts until someone presses the button.
The server takes `pageId` only while the experiment's variants are the control
alone, so the precondition expires the moment a variant is added. Add one during
the window and it is created under the OLD page, which the server was still on;
from then on no PATCH can move the experiment, and the card goes on showing a
page the experiment is not on. `revertRefusedPage$` only corrects that on a
`saveFailed`, which never arrives if Save Draft is never pressed.
The visible symptom was the round-trip: Edit/View built a URL for the page on
screen carrying a variant that only exists under the old one, so UVE loaded the
page and then could not find the variant on it.
Three changes:
- A confirmed pick is PATCHed on its own (`changePage$`), under the same
`canChangePage` rule the reducer and the server apply — an existing test
caught that omission. A partial body is what the endpoint expects; `setGoal`
sends `{ goals }` the same way.
- `pageChanging` gates the Variants card, and only that card, while the change
is in flight. Goal, Scheduling and Details do not depend on the page and stay
live.
- `buildVariantEditorLink` now takes the experiment's own `pageId` and refuses
when it disagrees with the page on screen. Required rather than optional, so
no caller can forget it. An unknown page is not a disagreement and is not
refused.
Verified in the browser: picking a page on a control-only draft fires
`PATCH /api/v1/experiments/<id>` with `{"pageId":"a9f3…"}` immediately, and the
experiment reads back on the new page with Save Draft never pressed. The
in-flight gate is covered by a held-open call in the store spec rather than in
the browser, where the request settles faster than it can be sampled.
…e panel footer
Two things from the same screen.
**The page card was lying.** The store resolves the experiment's page on load and
records why when it cannot — `pagePrefillError` — but nothing ever read that
state. So an experiment whose page had been deleted fell through to the card's
empty state, "Select a Page to test", which claims nobody ever chose one. The
editor was then refused at Preview/Edit ("This Variant's page cannot be opened
in the editor") with nothing on screen connecting the two. The card now reports
the reason instead of the empty state.
The two prefill strings said "the Page in this link", written for a bad
`?pageId=`/`?url=` deep link. They now read for both that and a loaded
experiment whose page is gone, which is the same fact either way.
**The panel footer had no fill.** Lara greys the header per colour scheme
(surface.50 light, surface.800 dark) and leaves the footer transparent, so a card
with actions at the bottom read as an unbounded continuation of its content. The
footer now takes the header's own emitted variable — not a surface token — so the
two cannot drift and dark mode follows for free. It is css rather than a token
override because Panel exposes no footer background token, only footer.padding.
App-wide, like the header it matches.
Verified in the browser: the card reports the unresolved page in place of the
empty copy, and the footer computes to rgb(248, 250, 252), the header's exact
fill. The reworded copy needs a backend rebuild to appear, so the string on
screen is still the old one.
…#37005) Adding a variant left the screen dirty for work that was already stored, and that is what broke "Edit variant". The variant endpoints persist on their own and answer with the recomputed proportion. The card mirrors that response into the form — but `savedFormValue` kept the pre-variant baseline, so the mirroring read as unsaved work. `loadSucceeded`, `createSucceeded` and `saveSucceeded` all settle the baseline; the three variant events did not. Two symptoms, one cause. Save Draft lit up for something the server already held, which is what made it look wrong. And `experimentsUnsavedChangesGuard` is a `CanDeactivateFn`: leaving Configure for the editor with a dirty form raises its confirm dialog, so "Edit variant" answered with "This experiment has changes that have not been saved" instead of opening UVE. The round-trip was blocked by a form that was not actually dirty. Only the proportion slice settles. `baselineOf` rebuilds the whole baseline from the server's experiment, which would call a name or a goal typed and never sent "saved" and lose it on the way out — the trap `saveSucceeded` already documents. A test pins that an unsent edit stays dirty across an added variant. Verified in the browser, the reported flow end to end: new experiment, pick a page, Save Draft, add a variant — the footer keeps its neutral hint instead of "You have unsaved changes" — then Edit variant lands on `#/edit-page/content?…&variantName=…&experimentId=…&mode=EDIT_MODE&experimentReturn=portlet` with the chip reading "Editing Var QA 2 Variant" and no confirm dialog, and the chip's arrow returns to `?section=variants`.
…content The page picker offers URL-mapped content — a `Destination` with a URL map renders as a page and can carry an experiment — but both queries that read a page back filtered `+contentType:htmlpageasset`. So one could be chosen and never resolved again. The experiment worked right after the pick, because the picker's own result fed the card directly, and broke on the next entry: the prefill found nothing, the card reported the page as missing, and Preview/Edit refused. On a page that was live the whole time. Found on `a1ab319a`, whose `pageId` is `c56e5030-fc88-480c-9b2e-4582fd762437`: zero hits with the content-type filter, and with it removed a working, live `Destination` at `/destinations/colorado`. Both queries now narrow by identifier alone. That is enough — the identifiers come from the experiments themselves, so whatever the contentlet turns out to be, it is the page the experiment stores. The Configure prefill and the list's Page column (and the editor link built from it) were both affected. Verified in the browser: reopening that experiment now shows "Colorado & The Rockies /destinations/colorado" instead of "The Page in this link could not be found". The Edit click that follows it is NOT verified — the dotAdmin session logged out mid-click and I could not sign back in.
`nx format:check` failed CI on one hand-wrapped line that Prettier keeps on a single line. The whole PR Test workflow reported red off it: only Frontend Unit Tests actually failed — on the `format-test` goal, not on a test — and every other job was cancelled in the cascade.
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
3 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
specs/37005-experiments-uve-integration/spec.md— the Spec-Kit specification for Experiments Portlet — UVE integration behind FEATURE_FLAG_EXPERIMENTS_PORTLET + variant Edit Content round-trip #37005. Spec only; the implementation lands in this same PR.The spec covers two halves of the work: switching the Universal Visual Editor's Experiments entry point to the new portlet behind a runtime switch, and completing the portlet's Variants card with the Edit Content round-trip into UVE.
Three premises in the issue did not survive contact with the code. Each is resolved in the spec as D1/D2/D3, and both #37005 and #37008 were corrected on GitHub to match.
D1 — the flag named in the issue is not free. The issue proposed reviving
FEATURE_FLAG_EXPERIMENTS, described as "declared, zero consumers today". That holds for the frontend enum entry only. The property is the live backend kill-switch for the whole Experiments feature —ConfigExperimentUtil.isExperimentEnabled(), gating experiment JS injection (ExperimentWebAPIImpl:338) and experiment resolution during page render (HTMLPageAssetRenderedAPIImpl:176,239). Setting it tofalse, the issue's intended default, would stop running experiments from serving to visitors. The spec requires a new dedicated switch,FEATURE_FLAG_EXPERIMENTS_PORTLET, shipped explicitlyfalse. The explicit default is required rather than optional:getFeatureFlags()andwithFlags()both map an absent flag to enabled, so a merely-declared flag ships on.D2 — the flag-on destination. The issue deferred it. The spec settles it as the site-wide list filtered to the page in hand, with defined behavior for zero, one and many experiments, and a filter the editor can clear.
D3 — no "edited" signal. The issue asked a variant's meta line to read "Edited in the Universal Visual Editor". No such signal exists on the variant model on either side of the wire, and the legacy per-page card shows no edit indication at all. The spec forbids inventing one. The Variants card as shipped by #37003 (PR #37064) makes no per-variant claim about content state, so FR-007 is a constraint to hold rather than a change to make: enabling Edit Content must add nothing that states or implies an edit state. The one string this work removes is the disabled button's tooltip,
experiments.configure.variants.edit-content.unavailable, which stops being true the moment FR-001 lands.Checklist
experiment.configure.edit.page.blockedcopy, which already reads as a full sentence.experiments.configure.variants.edit-content.unavailablewas the disabled button's "available in an upcoming release" tooltip and is now the refusal reason shown when a variant's page cannot be opened.ConfigExperimentUtilTestnow asserts that as a negative rather than leaving it a promise.Additional Info
Manual verification in progress. SC-002 (operator flips the switch in under a minute, no redeploy), SC-004 (round-trip under 30s of interaction, no address typed) and SC-003 (running experiments unaffected) are human measurements, not assertions — the mechanisms behind all three are unit-tested, the numbers are not. They are being measured on a live instance with the flag, together with the manual walkthrough in quickstart.md Scenarios 1–9. Results to follow here.
What shipped, by user story. US1 the variant round-trip (Edit Content opens the variant in UVE; the chip returns to the originating experiment). US2 the switch-off guarantee (every legacy destination byte-identical, enforced by destination-level tests). US3 the switch-on entry point (site-wide list filtered to the page, chip clearable, back-link, page-scoped empty state). US4 read-only for the control, any non-draft experiment, and a page locked by another user — with the lock reason now stated inline, not only on hover.
Spec review page: https://claude.ai/code/artifact/8deabd7c-6fb0-40a1-9241-7f5c111ec019
Depends on Experiments Portlet — Screen 1: portlet base + site-wide List #36989 (Screen 1), Experiments Portlet — Screen 2: Create/Update (/experiments/new + /:id/configuration) #37003 (Screen 2 — delivers the Variants card this work completes), Experiments Portlet — Screen 3: View Results #37004 (Screen 3).
Migration Experiments Portlet — migration: delete the old per-page experiments UI and remove the flag #37008 retires
FEATURE_FLAG_EXPERIMENTS_PORTLET, notFEATURE_FLAG_EXPERIMENTS.D2 confirmed as specified: with the switch on, the UVE Experiments button leads to the site-wide list filtered to the page in hand. FR-021a-c, US3 and SC-009 stand.
34 functional requirements, 9 success criteria, 4 user stories, 9 edge cases. No open
[NEEDS CLARIFICATION]markers.Test-tooling fix picked up while implementing this issue.
libs/portlets/dot-experiments/portlet/jest.config.tssetreporters: [['jest-junit', …]], which replaces Jest's default reporter instead of adding to it — so a failing run in that project printed a coverage table and a non-zero exit and no test results at all: no suite name, no failing test, no assertion. Diagnosing a Red run meant re-running everything throughnpx jest --reporters=default. Adding'default'back restores the console output; the JUnit XML is unchanged and still carries all 973 tests. Two other configs share the pattern and are deliberately left alone here —apps/dotcdn/jest.config.tsandlibs/portlets/dot-experiments/data-access/jest.config.ts.modeon the variant deep link sendsUVE_MODE, notDotPageMode. The two carry identical strings for EDIT/PREVIEW and diverge only at LIVE (ADMIN_MODEvsLIVE), and#getPageParams()validates?mode=againstObject.values(UVE_MODE)— so aDotPageMode.LIVEwould have been silently rewritten to EDIT. The planning notes claimed the opposite; corrected.CI note — this PR surfaces pre-existing breakage in
block-editor. Adding two constants and one enum entry tolibs/dotcms-modelsputs every dependent project into Nx's affected set, andblock-editorimports that lib in 31 files. Its own test suite has 37 failures —Unexpected "TestComponent" … is marked as standalone and can't be declared in any NgModule, an Angular-migration breakage in its own specs. Everyblock-editorfile is byte-identical toorigin/main, so this PR did not cause them; it makes the project affected, which is what runs the suite. The rot went unnoticed precisely becauseblock-editoris rarely in the affected set. A first local run appeared to pass because Nx served that task from cache —--skip-nx-cachereproduces the 37 failures reliably.dotcms-js,dotcms-webcomponentsanddotcms-block-editoralso enter the affected set and fail on a missingkarmamodule (karma is absent frompackage.jsonentirely), but all three carrytags: ["skip:test"], so CI's--exclude=tag:skip:testskips them.block-editorcarries no such tag.Every project this PR actually changes is green.
block-editordeserves its own issue; flagging rather than fixing it here.The variant return destination is resolved by origin, not by the switch. Branching on the switch alone makes FR-018 and FR-005/FR-027 mutually exclusive on a supported path: with the switch off an editor can still reach the portlet from the main navigation (FR-026) and still open a variant (FR-027), and a switch-only branch would return them to a legacy screen they never came from. The outbound leg carries an origin marker; the switch is the fallback for a deep-linked variant that has none.
Closes #37005