Conversation
12 tasks
Contributor
|
Claude finished @oidacra's task in 4m 55s —— View job Experiments panel reviewI re-read the changed files directly (the branch is stacked on Verdict: the three findings from the earlier review are all resolved, and I did not find a new introduced bug worth blocking on. The change is unusually well-documented — most non-obvious decisions carry a rationale comment that matches the code. Rechecked prior findingsResolved
New IssuesNone blocking. ExistingNone. Notes (non-blocking)
|
oidacra
marked this pull request as ready for review
September 9, 2026 21:28
oidacra
force-pushed
the
issue-37478-uve-experiments-panel
branch
from
September 9, 2026 21:29
db98ee0 to
07837ad
Compare
fmontes
previously requested changes
Sep 10, 2026
oidacra
force-pushed
the
issue-37478-uve-experiments-panel
branch
from
September 10, 2026 13:58
07837ad to
863f9ad
Compare
erickgonzalez
left a comment
erickgonzalez
left a comment
Member
There was a problem hiding this comment.
Just a nitpick:
D15 contradicts an already-approved spec
D15 says so itself: #37005 SC-002 promises an operator switch on this same flag, and FR-047/FR-048 rest on the opposite reframing. Two approved specs cannot both be right, and FR-048 is a hard requirement built on that reading.
Resolve before planning: amend #37005's SC-002 and get it re-approved, or drop FR-048 from this spec.
Member
Author
|
@erickgonzalez on D15 and #37005 — you were right that the spec said this, and the spec was wrong to say it. I went back to #37005's text and there is no contradiction to resolve, so neither of the two resolutions is needed. D15 is corrected instead. #37005 already hands the flag's removal to #37008. Two places in its own text:
Its US3 is also P2 and scoped to "someone who deliberately opts in". So on the flag's lifetime the two specs agree, and D15 was asserting a disagreement that is not in the text. SC-002 and FR-048 constrain different things. SC-002 measures how fast a flip takes effect ("under one minute, without a deployment or restart") — it says nothing about how long the flag lives or what may be built behind it. FR-048 constrains what may be built on either side. This spec keeps #37005's own uncached, fail-closed reader unchanged (FR-005): FR-048 also conflicts with nothing in #37005 — its FR-014, "the switch MUST NOT change the behavior of any capability other than the ... entry point", pushes the same way. What changed in the spec: D15's last paragraph no longer claims a contradiction, and instead records this check with the citations above. The note under US3 that said the reframing "needs reconciling with #37005's own spec" is corrected too. FR-047 and FR-048 stand as they were, and #37005 needs no amendment. On "resolve before planning" — the plan ran after this verification, and nothing in it turned on the outcome: the reader is #37005's, unchanged, and flag-off remains today's behavior exactly. The only planning artifact that moved was the note recording the check. |
12 tasks
Base automatically changed from
issue-37005-experiments-uve-integration
to
main
September 10, 2026 18:59
oidacra
added a commit
that referenced
this pull request
Sep 10, 2026
…ifetime D15 declared a contradiction with #37005 (UVE integration), reading its SC-002 as a promise that FEATURE_FLAG_EXPERIMENTS_PORTLET is a permanent operator switch. Checked against that spec's text: it says the opposite. Its Scope Note and its "Relationship to #37008" section both hand the flag's removal to #37008 (migration), together with its shipped default and its wiring, and its US3 is P2 and scoped to someone who deliberately opts in. The two specs agree on the flag's lifetime. SC-002 measures how fast a flip takes effect, not how long the flag lives or what may be built behind it. FR-005 keeps #37005's own uncached, fail-closed reader unchanged, so SC-002 goes on holding here. FR-048 conflicts with nothing in #37005 either — its FR-014, "the switch MUST NOT change the behavior of any capability other than the entry point", pushes the same way. D15's last paragraph now records that check with the citations instead of claiming the conflict, and the note under US3 no longer says the framing needs reconciling. FR-047 and FR-048 stand as they were; #37005 needs no amendment. Raised in review on #37485 (this PR).
oidacra
force-pushed
the
issue-37478-uve-experiments-panel
branch
from
September 10, 2026 21:13
863f9ad to
8718a32
Compare
github-actions
Bot
added
Area : Frontend
…canvas Spec for #37478. Keeps #37005's premise -- the editor's Experiments gesture means "the experiments for this page" -- and changes only where they are shown: beside the page instead of instead of it. The flow: the panel always opens on the page's list, whose empty state carries New Experiment; the page is fixed context in creation and configuration and says why; a variant's Preview or Edit Content leaves for the page and returns to the same experiment's configuration rather than to the list; and results render in the panel too, reached from configuration or from the toolbar's running-experiment badge. Three phases: list, configuration, results. Experiments are scoped to a page, not to a language version of one -- DotExperiment carries pageId and no language, and the page-scoped read narrows on pageId alone. The language #37005 carries from the editor is return context, not a filter, so creation stores none, a language change does not re-scope the panel, and the round trip carries it only to land the editor back on the version they came from. Grounded against this branch, and two claims are corrected. The list's row menu does not read push-publish environments off the route, so Push Publish and Add to Bundle need no route plumbing in a panel (D9). And the editor already fetches an experiment on every page load to feed an existing toolbar badge, so a "zero experiments requests" budget was never achievable -- the budget is that the panel adds none (FR-036). That badge is a second entry point that deep-links to the legacy route and ejects, so it is brought into scope (D14), which is what pulls results into the panel (D2). The flag is a development and QA gate, removed with the legacy screens by #37008 -- not a long-lived product switch. This contradicts #37005's SC-002, which promises operators can flip it live; recorded in D15 for resolution in that spec. 60 FRs, 17 SCs, 7 user stories, 15 resolved decisions, no open clarifications. Panel width for configuration and results stays an explicit open decision (O1).
…ifetime D15 declared a contradiction with #37005 (UVE integration), reading its SC-002 as a promise that FEATURE_FLAG_EXPERIMENTS_PORTLET is a permanent operator switch. Checked against that spec's text: it says the opposite. Its Scope Note and its "Relationship to #37008" section both hand the flag's removal to #37008 (migration), together with its shipped default and its wiring, and its US3 is P2 and scoped to someone who deliberately opts in. The two specs agree on the flag's lifetime. SC-002 measures how fast a flip takes effect, not how long the flag lives or what may be built behind it. FR-005 keeps #37005's own uncached, fail-closed reader unchanged, so SC-002 goes on holding here. FR-048 conflicts with nothing in #37005 either — its FR-014, "the switch MUST NOT change the behavior of any capability other than the entry point", pushes the same way. D15's last paragraph now records that check with the citations instead of claiming the conflict, and the note under US3 no longer says the framing needs reconciling. FR-047 and FR-048 stand as they were; #37005 needs no amendment. Raised in review on #37485 (this PR).
…y point Foundation for the Experiments panel (#37478), plus the US3 tests that describe the entry point it will hang from. DotExperimentsPanelStore holds what the editor is looking at: whether the panel is open, which of the four views it shows, and which experiment. It is provided by the UVE shell rather than by the panel, because the panel component is mounted and destroyed by the shell's `@if` and the variant round trip needs the state to survive a period during which the panel is closed. Leaving for a variant only changes the editor's query params, so the shell is never re-created; a browser reload does destroy it, which is the case the spec declines to define. close() and suspendForVariant() are two named methods rather than one with a flag. Collapsing them turns the round trip into a reset and the editor loses their place on every trip. The store lives in data-access, not beside the screens it serves: edit-ema reaches the portlet lib only through a dynamic import(), so Nx marks it lazy-loaded and forbids the static import a shell-level provider needs. data-access is the non-lazy boundary the two libs already share, and this keeps the portlet lib fully lazy — which is what the panel's load budget wants. The three US3 tests asserting the flag-on entry point are intentionally RED until the nav item stops carrying an href. The flag-off assertions pass now and must go on passing: they are characterization tests, and they are the regression net the flag exists to provide. Also adds the panel's message keys, a UVE editor page object the e2e harness did not have, and the flag-off e2e suite. Two of its scenarios are recorded as fixme with reasons: the toolbar badge needs a running experiment, and the Experiments portlet is opt-in so it sits in no layout on a stock instance.
…canvas The Experiments entry point stops being a destination (#37478, FR-001, FR-002). With FEATURE_FLAG_EXPERIMENTS_PORTLET on the navigation item carries no href, which is what EditEmaNavigationBarComponent already treats as an action, so the gesture opens a drawer beside the page instead of navigating away from it. The item also stops being highlighted, since $activeHref only matches items that have an href — the cost D1 accepts, and it needed no code. The panel is a p-drawer like the Edit Content side panel, with one deliberate difference: it is not modal and paints no mask. Edit Content is a full editing surface and blocking what is behind it is right; this panel must leave the editor usable, because the editor may navigate to another page while it is open and the panel re-scopes when they do. It is created from a dynamic import() the first time it opens and destroyed on close. @defer would read better but cannot be used: the experiments portlet lib is reached only through dynamic imports, so Nx marks it lazy-loaded and rejects any static import of it — and a @defer block still needs the component in imports:, which is one. import() is what @defer compiles to anyway. The split is the point: the whole lib stays out of what the editor loads until the panel is first opened. Only the list view is wired, and it is still a placeholder. The three US3 tests that were red now pass.
Expand/collapse with a persisted preference, Escape, click-outside, the transparent mask and the width transition — the drawer chrome the Edit Content side panel already has, copied deliberately rather than reinvented. Each of the four is wrong in a way that is invisible until someone hits it, and each has already been paid for once: - Click-outside compares against this drawer's own mask, never the shared .p-drawer-mask class, so another drawer's mask cannot close this panel. - Escape goes through DotKeyboardShortcutService rather than a document listener of this component's own. Two document listeners cannot arbitrate — preventDefault does not stop the other — so the editor behind would act on the same keypress that closed the panel. - hasOverlayAbove consumes Escape rather than declining it, so the key never falls through when something is above. - The storage reads and writes are best-effort, so a browser with storage disabled does not break the panel. The class docs say all of this in place, name the file it was copied from, and ask that neither panel be improved alone. The extraction is scoped there too: the natural moment is phase 2, when this panel gains the unsaved-changes veto that Edit Content already solved, and DotSidePanelNavController is the piece that makes it more than a file move. Where it differs: the mask is transparent because the page behind must stay visible. The consequence is that the editor is not clickable while the panel is open — clicking there closes it instead — which is a product call worth seeing in the running editor before it is settled.
The previous commit used it to withdraw the Escape claim without importing it, so the app build failed while lint and Jest stayed green — neither typechecks the way the Angular compiler does.
oidacra
force-pushed
the
issue-37478-uve-experiments-panel
branch
from
September 11, 2026 17:31
8718a32 to
5640b2a
Compare
…omments that promised @defer Three corrections /speckit-analyze surfaced, all of them the same shape: an artifact still describing a mechanism the code does not use. The shell's `.dot-ema-shell__body` went back to two grid tracks. The third was added for a panel that would sit inside the body, and the panel that shipped is a p-drawer with appendTo="body" — mounted outside it, overlaying the canvas rather than taking a column. Nothing has occupied that track since. A track no element occupies reads as intentional to the next person: it describes a layout the code does not have. The file is now byte-identical to main, so `dot-edit-ema-navigation-bar { grid-column-start: -1 }` resolves exactly where it did before this branch, and SC-007 is unaffected because the drawer never reflowed the canvas in the first place. The portlet barrel said the panel was exported "so the UVE shell can @defer it". The shell cannot. Nx marks this lib lazy-loaded, @nx/enforce-module-boundaries rejects any static import of it from edit-ema, and a @defer block still needs the component in `imports:` — a static import, so the rule fires on that too. The shell calls import() and createComponent instead. The export is right; the reason given for it was not, and it contradicted the shell's own comment three files away. The screens contract had the opposite error. It said results would load through "a second lazy import() inside the panel", generalising the shell→panel constraint to a boundary that is not one: results and the chart component live in this same lib, so the static import a @defer block needs never crosses anything the rule can see. @defer is available there, which is what tasks T077/T084 and the panel's own class docs already said. The contract was the outlier.
Contributor
|
Contributor
|
T021-T031, T035, T036. The panel stops being an empty drawer: it renders the portlet's own list screen, scoped to the page the editor is standing on. One screen, two presentations (FR-040). Panel mode is the presence of DotExperimentsPanelStore and nothing else — the portlet never provides it, the UVE shell always does — so neither the store nor the component grows a flag anyone has to remember to pass. Four concerns in the list store are bound to the browser address, and in the panel the address belongs to the editor: it names the page being edited, its language, its device and its preview mode. So the hydrate, the popstate subscription, the URL mirror and the site-change reload are all off there (FR-031, FR-032, FR-035). A list that hydrated from that address would read the editor's params as its own view state; one that wrote to it would push a history entry per filter change, and Back would step through the panel instead of leaving the page. onInit is split into named units for this: "which of these runs in the panel" is the question the file now has to answer. In their place the panel follows the editor's page. The re-scope effect reads only pageId, so a language change cannot reach it by construction rather than by a guard someone could later remove: an experiment belongs to a page, not to a language version of one, so refetching would re-request a provably identical set and resetting would discard the editor's place for no change in the answer. The language is still carried, as the return context the variant round trip needs (FR-034, FR-034a, D10). A page change resets the whole view state, expressed as the defaults rather than as a list of fields, so a field added later is reset by construction (FR-034b). The panel asks getAll(pageId) and skips the bulk page lookup: one request instead of one plus a lookup over every distinct page in the install (FR-010, D4, SC-005). That forces the site narrowing to be BYPASSED rather than left unfed. It reads pageInfoByPageId, which that lookup fills and the panel no longer runs, and it fails closed — so leaving it in place would drop every experiment for want of a host nobody asked for, and the screen would say "no experiments on this page" about a page that has them (FR-029, SC-008). There is nothing to narrow anyway: the server returned the experiments of this page. The compact layout is chosen by the mode flag alone, never by measured width. PANEL_WIDTH is a proportion of the viewport, so a width query would give the same build the seven-column table on a 1620px monitor and compact rows on a 1440px laptop (FR-041). The page column goes, because the panel is the page (FR-009). No breadcrumb: opening the panel is not navigation, and a crumb would put "Experiments" on the trail of a page the editor never left (FR-033, D8, SC-012). The portlet keeps all four address concerns, its table and its page column, with a regression test that fails if the panel branch is implemented by removing behaviour rather than by guarding it (FR-042).
…ot a copy of it The panel's list was a parallel set of flex rows: name, status, goal, schedule and the kebab, re-expressed as divs. It looked right and it was wrong. It silently dropped the paginator. Per page is 20, so on a page with more than twenty experiments the rest were simply unreachable from the panel — no control existed to reach them. It dropped lazy load, sorting and the rows-per-page control with it, and replaced the table's scrolling with a hand-rolled overflow. All four are the table's, and none of them was reimplemented. It also duplicated the row markup, which is the thing FR-040 and SC-015 exist to prevent: one implementation of each screen, two presentations. A second row template is not a presentation. The reasoning that led there does not survive inspection either. FR-041 rules out "the full-screen table layout", and the 81rem floor is what makes that layout unusable at panel width — but the floor is our own constant, not a property of p-table. Dropping the Page column (FR-009, 14rem) moves it to 64rem, and the panel clears that on any viewport the editor is usable on. So the panel now renders the same p-table the portlet does, with two differences that both follow from the column set: the Page column is absent, and the width floor and the skeleton row follow it. A skeleton wider than its header would put a cell outside the table while it loads. The mode flag still decides, and still alone — the floor is a scroll threshold, not a layout switch, and the T023a assertion is now stronger for it: the same build has to render the same columns at 1024px and at 2560px. Whether Goal, Variants and Modified earn their width in the panel is T004's call, and T004 is still open. Nothing here pre-empts it.
…ebuilds
Two assumptions about the editor's lifetime, both measured false in a running
instance and both invisible to the unit tests that encoded them.
The panel store moves to the route's providers, beside UVEStore. It was on the
shell component, on the reasoning that leaving for a variant only changes query
params so the component is never re-created. Instrumented, the opposite happens:
the store the panel suspended into is not the store the return reads.
`/edit-page` declares `data: { reuseRoute: false }` and route data is inherited,
so the custom reuse strategy tears down the whole subtree on any router
navigation beneath it. The shell dies on the way to the variant and the panel's
memory — which experiment, which screen — goes with it. The route's injector
outlives the component, which is why UVEStore is already there and the note
beside it says so.
The panel host becomes a signal query. Once the store survives, a rebuilt shell
finds the panel *already* open and `isOpen` never transitions — so the mount
effect ran once, before the view existed, returned early on a missing
`@ViewChild` and never looked again. The state said open and nothing was on
screen. Reading the host through `viewChild()` makes the effect re-run when it
resolves.
Both passed every unit test because the mocks asserted the assumption rather
than the behaviour: a store that never dies, a host that always exists.
…page The panel listed a page's experiments and then, on the first click, navigated away from the page it existed to keep. Every door out of it is now a view change. Eleven of them: the row and the kebab's Configure and Results, New Experiment and the empty state's offer, the configuration's back arrow and its header's Back and View Results, the results screen's two ways back and its Configuration button, and the swap the configure store performs after a POST. The list carries a sweep test asserting that *no* door navigates, written as a sweep rather than per door so a door added later fails there instead of shipping as the one that still ejects the editor. The twelfth still navigates, and must: editing a variant means going to the editor on that variant. What the panel adds is `suspendForVariant()` on the way out, so the trip is a round trip. The panel now renders all four surfaces. Creation is the configuration screen with no experiment yet — same component, `experimentId` being null is what puts it in that mode — and results sit behind their own `@defer` so chart.js loads only when they are opened. The configure store follows `panel.experimentId()` instead of `route.paramMap`, and hands a created experiment to the panel rather than swapping a URL that does not exist there. The Page card stops offering to change the page, and says why. In the panel the page is not a choice: it is the page open in the editor, and the panel is scoped to it. Letting it be changed would point the experiment at a page the editor is not looking at, and leave the panel listing one page's experiments while configuring another's. A control that is simply missing reads as broken, so the card carries a line explaining that the experiment runs on the page being edited. Two things the panel had been quietly getting wrong, both of them from route resolvers it has no routes to run. The Scheduling card was falling back to its 7-and-90-day defaults instead of the install's configured window, silently; the panel now resolves those properties at its own door and the screen prefers that answer. And results was asking for analytics health a second time, when the list's own gate had already answered — that answer now travels through the panel store, which is what FR-027 and SC-005 ask for.
…thout a jump The round trip closes: the panel suspends on the way out, the chip brings it back on the same experiment's configuration, at the Variants card the trip started from. Most of this commit is what it took to make that true in a running editor rather than in a mock. The chip's single button now means what it means everywhere else in that bar — get the editor out of the state they are in — and reopens the panel, which is the two halves FR-023 asks for. It answers the suspended panel first, before the `pageExperiment()` guard, because that signal is null in exactly the case this branch exists for: a DRAFT experiment is not running on the page, so once the editor is back on the original there is no experiment on the page asset. The guard was swallowing the click and the chip did nothing at all. It leaves the variant by loading, not by routing. `/edit-page` declares `reuseRoute: false` and route data is inherited, so a `router.navigate` there rebuilds the whole editor — toolbar, canvas and a new iframe element — which the editor sees as a jump for a page that only needed its content swapped. UVE never routes to change what it shows: it loads, and the shell mirrors the params through `Location.go`. Same path the persona and language selectors take. And it leaves only when there is something to leave. `variantName` is present as `DEFAULT` while previewing the CONTROL, so reading it as presence reloaded the page the editor was already standing on. The fallback, for a trip that began in the full-screen portlet or lost its memory to a reload, rebuilds the return from the address — and names the experiment the editor was *previewing* rather than `pageExperiment()`, which answers "what is running on this page" and commonly a different experiment entirely. That reopened the panel on one the editor never clicked. The origin marker stops deciding whether the chip exists, where it was lying: `buildVariantEditorLink` writes `experimentReturn=portlet` unconditionally, so it claims a portlet origin for trips that began in the panel. With the switch on the chip keys on the experiment being named by the address, which is true. With it off the marker still decides, unchanged — the legacy in-UVE screens send the same `experimentId` and must not gain a chip they never had. The marker is also cleared on the way back: once the editor has returned there is no trip left for it to describe. The running-experiment tag becomes an action when the panel exists, and goes straight to results — it announces what is running, and the only question it raises is how it is doing. It drops its `routerLink` rather than suppressing it, because the directive would navigate on the same click that opens the panel. With the switch off it stays the legacy destination FR-017 of #37005 constrains. The page re-scope stops reacting to an unloaded page: `pageAsset` has no identifier while a fetch is out, and treating that gap as a page change wiped the view the return had just restored.
…ly that Two fields had drifted into it that were never view state, and one of them was the only duplicated state in the feature. `healthStatus` was held in the list store *and* copied to the panel so results could read it. `configProps` was fetched by the panel component on behalf of a screen it does not own. Both are install-level answers, and both are there in the portlet too — supplied by route resolvers, not by any store. So each screen now asks for its own, through the same service its resolver uses: results calls `healthCheck()` once when it opens, configuration reads the two duration properties when no route resolved them. That is exactly what the portlet does per screen, so it is parity rather than a regression, and it deletes a relay effect in the list store, a fetch in the panel component, two setters written by strangers, and the `viewState()` split that existed only to protect those two fields from a reset. What is left is five fields of one kind: which surface is open, for which experiment, at which section, plus one bit for a trip still in flight. The class docs now say what that is. The panel store is the panel's address bar — the portlet says which screen is open, for which experiment and at which section in its URL, and its screens read it back off the route; the panel has no URL to say it in, so it says it here. Which makes this the portlet's own layering rather than a second one: the list, configure and results stores are untouched and still own their data, their forms and their requests. All that changes for them is where their route param comes from. Nobody calls the Router a second store. And the rule that follows, which is what this commit applies: anything a screen can ask for itself does not live here.
…h worlds The same fork was written sixteen times in eleven files: check for the panel store, change a view and return, otherwise navigate. It is the kind of duplication that does not merely repeat — it lets a destination be taught to the panel in one place and left navigating in another, which is exactly how this feature kept ejecting the editor from doors nobody had looked at. `DotExperimentsRouter` holds it once. Named for what it is: the panel store is the panel's address, and this is what moves between addresses — Angular's Router for the portlet, that store for the panel. Consumers inject it as `#experimentsRouter` so the two never read as the same thing at a glance. Five destinations, and each screen now says where it wants to go rather than how to get there. When the entry-point switch is retired the fork is deleted here and nowhere else. Not `providedIn: 'root'`: the panel store is provided by the UVE route, so only an injector beneath it can see it, and a root-provided router would always resolve `null` and quietly navigate the editor away. Each screen provides it and its cards and headers inherit the instance. Two things fell out that are worth more than the deduplication. The configure store no longer navigates at all. Injecting the router into it made a cycle — store to router to panel store — so the post-create URL swap moved to the component, where the rest of this screen's navigation already lived. The rule the portlet follows everywhere else, that stores hold state and components move between screens, is now true without an exception, and `Router` is gone from that file. And `listReturnParams` stops leaking. Six components knew that a destination carries `?pageId=` so the next screen can find its way back — a portlet contract, known by components that are sometimes not in the portlet. Only the list still passes a narrowing, because it holds its own in its store rather than on the route it is about to leave. No test changed meaning: the router is provided real in every spec, so they still assert what the editor sees — `showResults(id)` or `navigate([...])` — rather than that a method was called. Two moved from the configure store's spec to the component's, following the behaviour. The router's own spec adds the sweep that matters: no destination it offers ever reaches the Angular router in panel mode.
The panel was created by hand: a `ViewContainerRef` found through a signal query, an effect that cleared it on close, a length check so it was not created twice, and a re-check after the `await` because the editor can dismiss the panel while its chunk is still in flight. Four mechanisms, all of them doing what a structural directive does. Only the `import()` has to be imperative, and that is forced twice over. The experiments portlet lib is reached only through dynamic imports, so Nx marks it lazy-loaded and `@nx/enforce-module-boundaries` rejects the static import a `@defer` block still needs in `imports:`. And past that rule there would be no chunk anyway: `@defer` would reach the component through the lib's barrel, and a bundler keeps a barrel's exports together, so the whole lib would land in the editor's bundle — the opposite of the split. Three screens, four stores and chart.js stay out of what the editor loads until the panel is first opened. So the effect now resolves the component class and nothing else, and `NgComponentOutlet` under an `@if` mounts and destroys it. Closing still leaves nothing running, and reopening no longer re-fetches the chunk. The post-`await` re-check is gone because there is nothing left to defend: if the editor closed the panel while the chunk was loading, the `@if` simply never renders it. The host query goes with it, and so does the paragraph explaining why it had to be tracked. A rebuilt shell that finds the panel already open — what the variant round trip does on its way back — used to depend on the effect seeing the host resolve. The template has no such timing to get wrong. Verified in a running editor: open, close, reopen, and the full variant trip back into Configure.
Clicking the back arrow reopened the panel and left the blue bar standing above it — a bar offering a way back that had already been taken. Two causes, and the second is why it happened on the control and not on a real variant. The chip read `ActivatedRoute.snapshot.queryParams`, which is the frozen half of the address. UVE never routes to change what it is showing: it loads, and the shell mirrors the new params into the URL with `Location.go`, which does not notify the router. So the snapshot holds whatever the navigation that built this toolbar carried, forever. A chip that has to *disappear* — on a toolbar that stays mounted while the params are cleared underneath it — cannot be derived from it. It now reads `pageParams()`, the half that stays current and the one UVE actually writes. The comment above it already predicted this: it said the snapshot was safe only because nothing rewrites those params on a live instance, and named the flow that would break it. Returning to the panel is that flow. And leaving the control cleared nothing at all. `variantName=DEFAULT` is present but is not a variant, so the early return fired — rightly, since there is no page to fetch and `pageLoad` there would restart UVE for nothing — and carried the experiment params out with it. They have to go regardless: they are what the chip is made of. `pageUpdateParams` is the load-free half of `pageLoad`, so the params drop and the address follows without the canvas being touched. A real variant was never the problem. There the bar is the store's own, built from params that `pageLoad` has already cleared. Also removes two duplicated doc blocks in `handleInfoDisplayAction`: one pasted verbatim twice, and one orphan left over describing an approach that now lives in `#leaveTheVariant`. Both were flagged in review. The spec grows a `setAddress` helper, because a test that writes only one half of the address describes an editor that never exists — and that is exactly what let this ship. Both store writers now merge into `pageParams`, as the real ones do, so the new test watches the bar leave rather than watching a method get called.
…its code Six files were committed unformatted, which is the only thing red on this PR: `format-test` fails, and the E2E shards that look failed were cancelled by it rather than broken. Every other change here is whitespace. The exception is `dot-experiments-results`. Two doc blocks were stacked on `#panelHealthStatus`, and the first was not merely redundant — it described the merged `#healthStatus` computed and claimed this screen does *not* ask for the health status because the panel's list already did. It does ask, on the very next line. The block moves to the field it was written for, without the sentence the code contradicts.
| * on the next open. Nothing here has to survive the `await`: if the editor dismissed the panel | ||
| * while it was in flight, the template simply never renders it. | ||
| */ | ||
| readonly $experimentsPanelLoader = effect(() => { |
Member
There was a problem hiding this comment.
This dynamic import() has no error handling. If the chunk fails to load (stale deploy referencing a hash the CDN no longer has, a transient network failure, or a script error inside the experiments bundle), the promise rejects silently: $experimentsPanelComponent stays null forever, the @if in the template never renders anything, and the editor is left with a nav item that silently does nothing — no toast, no log, no monitoring signal, nothing to debug from.
Suggest wrapping the await import(...) in a try/catch that logs the error, surfaces a toast to the user, and calls this.experimentsPanel.close() so the panel isn't left stuck isOpen: true with nothing to show for it.
| experiments.panel.action.collapse=Collapse the Experiments panel | ||
| experiments.panel.shortcut.close=Close the Experiments panel | ||
| experiments.panel.empty.description=Nothing is being tested on this Page yet. | ||
| experiments.panel.action.view-all=View all Experiments |
Member
There was a problem hiding this comment.
These two keys (experiments.panel.action.view-all / .view-all.tooltip) are added here but never referenced by any component in this PR — I grepped the full diff and found no view-all/viewAll usage outside this file and the spec docs.
Per the linked issue's acceptance criteria, the panel needs an explicit, honest way out to the full /experiments portlet (this is FR-021c's 'clearable' requirement, and the spec's own D6 decision: a link opening the portlet unfiltered in a new tab so the page stays open here). Right now the copy is prepared but the actual button/link isn't wired up in dot-experiments-panel.component.html/.ts.
Can you add the missing action, or drop these keys if it's intentionally deferred to a follow-up?
| }); | ||
| }); | ||
|
|
||
| describe('returnFromVariant()', () => { |
Member
There was a problem hiding this comment.
None of the 4 tests in this describe('returnFromVariant()', ...) block call initStore() — they read directly from the outer-scope store, which is left in whatever state the previous describe('re-scoping on the editor page') block's last test happened to leave it in (view: 'configure', experimentId: 'exp-9', suspendedForVariant: false).
For example, 'should return from a panel that was never suspended' asserts expect(store.suspendedForVariant()).toBe(false) as if it were a verified precondition — but that's only true because of leftover state from a sibling block, not anything this test itself sets up. It also doesn't match the block's own doc comment above, which claims these tests start 'from the worst starting points on purpose... a panel that was never suspended' — no such starting point is actually constructed.
This makes the whole block order-dependent: reordering tests, filtering, .only, or inserting a new describe above this one could silently break these tests, or make them silently pass for the wrong reason. Can you add initStore() (with explicit setup per test, e.g. plain initStore() alone for the 'never suspended' case) at the top of each it here, the way every other block in this file already does?
… to it Opening an experiment from the full-screen portlet and pressing Preview lands the editor in the UVE; the chip back then opened the panel on the **list** instead of the configuration it came from. The way back was being built correctly and thrown away a moment later. Re-scoping means "the editor moved to another page", and that needs a page to have moved *from*. Coming from the portlet the shell is built from nothing, so `setContext` runs while the page is still loading and hands over a null; the first real identifier to arrive was then read as a change, and the reset took the destination the return had just set. Inside the UVE it never showed, because there the store is created on a shell that already has its page. The first identifier a store ever sees is the page it is already on. It is adopted now, and only a second, different one re-scopes. Found by reproducing the trip at store level rather than by reading: the ordering that fails and the ordering that does not differ by one `flushEffects`, and both are now tests.
…low it Leaving a variant left the editor with no mode selected at all, or stuck in Preview, and the address missing `mode=` entirely. Two causes, in the two halves of the same transition. `CLEARED_VARIANT_PARAMS` carried `mode: null`, which only ever meant "drop it from the address and let the shell's default put it back". That held while these params were the `queryParams` of a `router.navigate`: the shell re-read the route, found no mode and applied `UVE_MODE.EDIT`. They now go to `pageLoad`, which writes straight into `pageParams` and is mirrored to the address with `Location.go` — a write the router never hears, so `#getPageParams` never runs and the default never lands. The null survived as a null. `EDIT` is not a new choice: it is where a return has always landed, since dropping the param and re-reading the route produced exactly that, including when the editor left from LIVE. The control's return keeps its load-free shortcut, but only in the shape where its premise holds — on the control *and* already in EDIT. Previewing the control is a mode change, and a mode change is not a param change: `PREVIEW` and `EDIT` render different canvases, so patching the param without loading would leave the toolbar claiming one mode over an iframe still showing the other. And the selector was copying the mode instead of deriving it. Seeded once in `ngOnInit`, it held only because every mode change either came from the select itself or from a navigation that rebuilt the toolbar; the way back is neither, so it went on naming the mode the editor had left while the address and the store had already moved on. `linkedSignal` keeps the two-way binding the template needs and re-derives when the store's mode moves underneath it. Both halves are mutation-checked, and the whole trip was walked in a running editor: portlet, draft, Variants, Preview, back — configuration, chip gone, address clean, selector on Draft.
The panel's code is fetched on first open, and that fetch had no failure path. A deploy pointing at a hash the CDN has already dropped fails exactly there: the class stayed null, the `@if` rendered nothing, and the editor was left with an Experiments item that did nothing at all — no panel, no message, nothing in the console to chase. Raised in review. It now logs, raises a toast, and closes the panel. The close is part of the report rather than tidying up: the store would otherwise still say open, and the loader only fires on that transition, so a second click would be swallowed and the editor could not even retry. Two tests, and the first fills a gap this file had all along — nothing here ever asserted the panel reaches the screen. These specs watched the store and stopped there, so the whole mounting path was covered only by opening the editor and looking at it. The failure is driven by a throwing getter rather than a rejecting module factory: the factory's result is cached after the first call, and a test that depends on which of its siblings ran first is worth less than no test.
Configuration writes only when Save Draft is pressed. In the portlet a route guard stands between a stray click and losing everything typed since; in the panel there was nothing. Closing it — the X, the mask, Escape — is a click, not a navigation, and `canDeactivate` sees none of them. Back inside the panel changes a store field rather than a URL, so it saw that either. The work went without a word. The prompt moves out of the guard into `confirmLeavingUnsavedChanges`, and the guard keeps only what is genuinely routing: the creation redirect, which exists solely as a pair of URLs. Same question, same words, both worlds — a second dialog worded almost the same would be worse than none. The helper takes a structural source rather than the component: three callers reach it from three places in the tree and share only two members. The `ConfirmationService` matters as much as the store — it is provided by the Configure component, so an injector below it resolves the instance whose `<p-confirmDialog>` is actually rendered, and one resolved from anywhere else would ask into a dialog nobody can see. The panel finds the screen through `viewChild`: `undefined` on every other view, which is exactly the test it needs, because only configuration can have anything to lose. The header asks only in panel mode — in the portlet the navigation raises the guard, and asking twice is the bug. A dismissal is not an answer. PrimeNG funnels the X, ESC and a mask click through the same callback as the secondary button, and reading them alike would discard the work on a mis-click. Asserted on both callers. Walked in a running editor: all three closes are instant with nothing unsaved; with unsaved work each asks, Keep Editing and the dialog's X stay, Escape asks rather than closing, and only Discard closes.
The panel answers "what is running on this page". This is the door to "everything", and it must not cost the editor the page they are standing on. A real anchor, not a handler calling `window.open`. "Opens in a new tab" is a promise to the editor rather than to the code: an anchor honours cmd-click and middle-click, survives a popup blocker and shows where it goes on hover. A handler delivers none of that and would still satisfy a `window.open` spy. Unfiltered, deliberately (D6): in a page-scoped panel there is no filter to clear, so carrying `pageId` would narrow the one destination whose whole point is that it does not narrow. The four tests for this were written first and sat red; they pass now. Its label turned out to be `open-portlet` rather than the `view-all` pair added ahead of the control — those had sat unreferenced through review and were removed rather than shipped as dead copy, which review also caught. Also adds the two keys the chunk-failure toast needs, and asserts what the results screen's own `@defer` is for: opening the panel on the list, or on configuration, must not bring the charting library with it.
…its header
Four surfaces the editor reads as a single column began at four different x
positions: the drawer header takes PrimeNG's `{overlay.modal.padding}`, the
list's toolbar carried `px-3`, the table's outer cells whatever the datatable
preset gives, and the configuration header `px-8`. Nothing looked broken; it
just never looked deliberate, and the experiment's title sat a centimetre right
of the panel's own.
All four now share one number. Only in the panel — the portlet's screens sit in
a page with their own gutter and keep what they have.
The drawer header also gains the rule it never had. PrimeNG gives
`p-drawer-header` padding and no border, so every screen below it started flush
against the panel's title with nothing marking where the chrome ended and the
screen began. Drawn once on the drawer rather than per screen, so the four
cannot disagree about it.
And the configuration header's kebab was 7px short of the edge: an empty
`<p-menu>` sitting in the flex row as a zero-width item, dragging the row's gap
with it. `appendTo="body"` moves the popup, not the host. Hidden now — the popup
is positioned against the button passed to `toggle($event)`, so the host's
display has nothing to do with where it opens.
Measured rather than eyeballed, in a running editor: left edges at 19px, right
edges at 18px, on both the list and the configuration.
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
4 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.
Closes #37478
With
FEATURE_FLAG_EXPERIMENTS_PORTLETon, the Universal Visual Editor's Experiments entry point currently ejects the editor: thesciencenav item navigates to the portlet, the iframe is torn down, and the editor gets back to the page through the breadcrumb. This keeps the editor on the page — the experiments for the page in hand render in a panel beside the canvas, behind the same flag. No new switch, no second rollout.It also closes what #37005 (flagged UVE entry point) left half-done. Its FR-021c asked for the page filter to be "visible and clearable"; the filter is applied but has no affordance to clear it, and clearing it would be a dead end — the portlet is opt-in, so on a stock instance the admin menu carries no Experiments entry to return to. In a page-scoped panel there is no filter to clear: the panel is the page's scope by construction, and "clearable" becomes an explicit link out to the full portlet.
Proposed Changes
The flow, end to end
Two phases, one PR
Phase 1 ships usable on its own, and carries the shared plumbing below — phase 2 changes none of it.
The load-bearing work
Location.go(…{queryParamsHandling:'merge'})and re-hydrates on popstate. UVE writes its own params with no merge, replacing the whole query string. Two writers on one address: the panel's state gets silently dropped by UVE's next view change. In the panel the view state lives in memory, seeded frompageId/languageId. This is the largest piece of work here, and it is a store change.getAllUnfiltered()— every experiment on every site — then resolves each distinctpageIdthrough a bulkhtmlpageassetlookup and narrows client-side. The panel calls the existinggetAll(pageId)and skips the lookup, because UVE already holdspageAsset(). Three requests plus a full-set payload become one. Same swap point as Experiments Portlet — swap the List to the server-side contract #37007 (server-side list contract).UVE_FEATURE_FLAGSwould invert the default —withFlagsmapsFEATURE_FLAG_NOT_FOUNDtotrue. The dedicatedreadExperimentsPortletSwitchstays, and the shell already makes that request for the nav item, so the panel adds none.LIST_TABLE_STYLE'smin-width: 81rem(1296px across seven columns) cannot render at panel width, so the layout is what becomes mode-dependent; the state and the rules are shared. The page column drops in panel mode.Must not break
experiments/{pageId}destination, untouched — and the flag still fails closed on a missing key and on a failed read./experiments, reachable and unfiltered from the main navigation, keeping its table, its columns and its address-backed view state.Additional Info
Stacked on
issue-37005-experiments-uve-integration(#37256), which is still open — that branch is the base, notmain.The specification is the first commit here (
specs/37478-uve-experiments-panel/): 47 functional requirements, 16 success criteria, 7 user stories, 13 resolved decisions. Implementation follows in this same PR.One correction to the issue. #37478 item 5 states that the list route's
DotPushPublishEnvironmentsResolveris read by the row menu, and that a panel mounted outside the router would break Push Publish silently. It is not read —dot-experiments-list.component.tsnever touches route data, Push Publish opens throughDotPushPublishDialogServicewith only anassetIdentifier, anddot-add-to-bundlefetches its own bundles. That resolver is a prefetch nothing consumes (contrastdot-locales-list.component.ts:75, which does read it). Both actions therefore work from a panel row with no route plumbing. Recorded as decision D9; the issue has been corrected. The health-gate half of item 5 does hold and is covered.One decision deliberately left open (O1). The panel's width for phase 2. Configuration is a multi-card form laid out for a full-width column and its Variants card is a table, so it does not fit phase 1's width. Two candidates — a wider panel for that screen only (the Edit Content side panel already varies its own width, 80% to 100%), or a single-column reflow of the cards. It blocks nothing in phase 1.
Related: #37007 (server-side list contract, same swap point), #37008 (migration — this makes it simpler: the
experimentsrouted child and the legacy branch of the nav item both go, and no full-page eject survives). Epic: #36763 (Experiments: A/B Testing v2).Checklist