docs(experiments): specification for the Experiments portlet - #37144
Open
oidacra wants to merge 48 commits into
Open
docs(experiments): specification for the Experiments portlet#37144oidacra wants to merge 48 commits into
oidacra wants to merge 48 commits into
Conversation
2 tasks
Contributor
|
Claude finished @oidacra's task in 1m 30s —— View job Review: Experiments Portlet spec
This is a documentation-only PR (one new spec file, +736). There's no code, build config, or test to break, so the review is about internal consistency and whether the concrete, verifiable claims match the tree. I spot-checked the load-bearing ones. Verified accurate:
New Issues
Nothing here is blocking. The spec is accurate against the code I could verify, and the |
oidacra
force-pushed
the
issue-36987-experiments-portlet-spec
branch
2 times, most recently
from
August 20, 2026 18:17
5305f6a to
64ee5b7
Compare
erickgonzalez
previously requested changes
Aug 20, 2026
oidacra
force-pushed
the
issue-36987-experiments-portlet-spec
branch
from
August 20, 2026 19:15
64ee5b7 to
de81d1b
Compare
…e endpoint DotPagesBrowserService wraps /api/v1/page/search, folder search and an ES-search page-lock lookup for the standalone Configure screen, which has no UVE resolver chain to lean on. DotExperimentsService gains setName, mirroring setDescription over the generic experiments PATCH.
…ed autosave Signal Store Events, same shape as the list store: page-intent and API-outcome event groups, reducers per card, and one debounced switchMap handler per PATCH field group so edits collapse per group without cross-group merging. Creation is the autosave: one POST as soon as name+page exist, then a relative replaceUrl swap to /:id/configuration. targetingConditions never enters an outgoing payload. Skipped autosaves settle through an explicit event so $isAutosaving cannot stick. The 8 start-gated validation rules, transitions (start/stop/cancelSchedule/ abort over the same endpoint) and non-DRAFT locking, SCHEDULED included, are lifted from the legacy configuration store.
…wo dialogs Routed screen per the approved design: fixed header (back, title, status, kebab gated by AllowedActionsByExperimentStatus), locked banner, scrolling 1040px card column and pinned footer whose Start button is never disabled. One component per card - Details, Goal (radio types plus backend-validated condition operators), Page (immutable after creation, prefill error inline), Variants (inplace rename, copy variant URL, lock awareness, split evenly, n/max cap) and Scheduling (p-datepicker with time plus min/max duration bounds). Select A Page (folder tree, page table, pages with a live experiment greyed) and Add Variant dialogs. All form state is Angular signal forms; Jest specs accompany every component.
…ide list actions One UrlMatcher-based route config serves both paths so the post-creation replaceUrl swap reuses the shell instance instead of tearing it down mid-autosave; the experiments mount point drops reuseRoute: false for the same reason, mirroring the /content precedent. The list's New Experiment button comes alive and Configure leads each row's kebab.
89 keys covering the five cards, both dialogs, header/footer, locked banners and validation copy, following the list screen's naming.
The matcher, the list's navigation commands and the store's post-creation replaceUrl all spelled 'new'/'configuration' independently; one rename now touches one file.
…tants Pure functions move to util/dot-experiments-configure.util.ts and the i18n keys plus weight constants to shared/constants.ts, matching the list's layout. Kills three duplicate '/experiments' literals and a second TOTAL_WEIGHT along the way.
…PATCH
PATCH /api/v1/experiments/{id} applies every key of its body atomically,
so the per-field-group event pairs collapse into formEdited plus one
save triple over a generic DotExperimentsService.patch. The reducer
accumulates the diff; a single debounced switchMap flushes it, holding
back what the backend would reject (weights not summing 100 construct-
time-throw on TrafficProportion) and settling only the keys each
response actually carried, so mid-fix weights never snap back. SAVING
now marks the flight alone via saveRequested. Validation docs cite the
verified backend enforcement points.
…dTree slices One ConfigureFormModel and one form() with the schema inline; the cards take their subtree as input.required<FieldTree<Slice>> and bind leaves with [formField], with goal/scheduling rules composed via apply() from schemas the cards export. Hydration happens once per experiment in the shell, and a single bindFormAutosave diffs the root model. required() stays out of the schema deliberately: it reaches the DOM as the native attribute and would paint empty fields red on first render (AC28). Also the visual review pass: screen-enter fade on both screens, the traffic p-slider freed from a full-width input group, textarea styled via pInputText (bare p-textarea classes load no CSS without the directive, which crashes against signal forms), circular back button, wider control gaps, a synthetic Original/CONTROL row while the draft does not exist yet, and UVE's 3px indeterminate bar for the flight of each save.
… as validate()
variantWeights becomes a form slice with applyEach bounds per row and a
cross-field validate() reporting {kind: weightsTotal} — the same literal
the Start rules use, so the live warning bar and the reveal-on-start read
one truth. The card's warning and total read the slice; splitEvenly stops
being a store event (the card writes splitWeightsEvenly into the slice,
keeping SPLIT_EVENLY on the wire so addVariant still redistributes
server-side). The slice reseeds only when the variant identity set
changes, so save echoes never clobber weights mid-edit, and the wire
guard holding back sums that are not 100 stays untouched. Weight is
number|null: a cleared native number input writes null, not 0.
FormValueControl<string> per the custom-controls guide: value is the group's selection (the contract reserves that name, so the card's own option rides an option input), checked derives from their equality, and '' is the no-selection sentinel. Host is the radio — role, aria-checked, tabindex and keyboard selection — with the goal card's approved visuals. Signal forms only by decision: a ControlValueAccessor would even shadow the signal-native path, since FormField resolves CVA first. The goal card renders its four types with it, keeping the atomic slice write on select.
The circle is now p-radioButton's, rebuilt rather than reused: a 1.5rem box with a 1px border that fills with primary when checked, holding a 1rem primary.contrast dot that scales between 0.1 and 1 over form.field.transition.duration. Those are a real radio's rendered values, measured in the running app, so a radio drawn as a card now looks like every other radio in dotCMS — and gaining or losing the selection animates in both directions instead of switching in a single frame. p-radioButton itself cannot be reused here: it resolves NgControl from the injector without `optional`, so mounting one with no reactive or template form binding throws NG0201. Reusing it would drag NgControl into a control that is signal forms only, and nest a second focusable radio inside a host that is already role="radio". The card's own outline follows form.field as well, because a radio card is a control and not a container: the same 6px radius and border colour as the pInputText beside it. Nothing points at --p-radiobutton-* or --p-card-* — PrimeNG emits a component's variables together with its stylesheet, so both families are undefined on a screen with no radio button and no card mounted.
The surface, radius, border, padding and shadow are no longer restated in the component: they come from `components.card` in the preset, and follow it when it changes. The card now renders at the theme's 8px radius with its 1px gray.300 border and 1rem body padding, and mounting it also brings the `--p-card-*` tokens and the `.p-card` stylesheet onto the screen, which were absent before. `p-card` puts `.p-card` on its own host element, so the radio semantics stay on `<dot-radio-card>` one level out and the host is left a bare block the card fills. The selection accent is the only thing painted over the theme, and it needs `!` to land: PrimeNG injects component CSS outside any cascade layer while Tailwind's utilities live in `@layer utilities`, and unlayered declarations win regardless of specificity. The radio circle stays hand-built — `p-radioButton` resolves `NgControl` without `optional` and throws NG0201 with no form binding — but it is built from a real radio's measured values, so it reads as a PrimeNG radio.
The reason was buried next to the class constants it justifies. It belongs in the component doc, where a reader looks before reaching for p-radioButton again: its `onInit` calls `this.injector.get(NgControl)` with no `optional`, so one rendered with no ngModel, formControl or formControlName throws NG0201 — confirmed by mounting a bare one. Reusing it would mean an internal reactive-forms control existing only to paint a circle, inside a control whose contract is signal forms, plus a second focusable radio nested in a host that is already role="radio". Names the condition that would make it worth revisiting, so the decision does not have to be re-derived.
None of the five sections draws its own card any more: Details, Goal, Page, Variants and Scheduling each dropped `rounded-lg border border-surface-200 bg-white p-6` for a `p-card`, so their surface, radius, border, padding and shadow all come from `components.card` in the preset and follow it when it changes. They were rendering at 7px with a `surface-200` border while the theme's card is 8px with a `gray.300` one, which is exactly the drift this removes — the interior padding is now the theme's `1rem` rather than the template's `1.5rem`. The `<section>` element stays, one level in, so the card is chrome and the sections keep their grouping semantics and their `data-testid`.
Cuts the usage example, the card-chrome and animation paragraphs and the per-member restatements — the template and the class names carry those. Keeps the four things a reader cannot derive: that each card is one radio rather than a select, that it is signal forms only, why the circle is not a p-radioButton (NG0201), and why the accent needs the important suffix.
…s strings The class no longer decides how anything looks. `aria-checked` and `aria-disabled` were already on the host as the radio's semantics, so the stylesheet keys off them and the six class-name constants, the three class computeds and the host `[class]` binding all go away — including the two `!important` utilities: a component stylesheet is unlayered and scoped, so `p-card[_ngcontent]` outranks PrimeNG's `.p-card` on specificity where a Tailwind utility, sitting in `@layer utilities`, could not. Verified in the browser: the accent, the circle and the 0.1→1 dot all still land, sampled mid-transition. Real CSS also states the radio's geometry in the units the theme uses, so the comments explaining `border-(--p-form-field-border-color)`-style strings are gone with the strings. The template keeps Tailwind for layout and type, where nothing competes with it. Three assertions on class strings go with them: the appearance they stood for is now CSS keyed on an attribute the tests already assert, and jsdom computes no styles.
…ither forms API One implementation now serves every caller: reactive forms with `[formControl]`, templates with `[(ngModel)]`, signal forms with `[formField]` through the interop bridge, and no form at all with `[(value)]`. It replaces `FormValueControl`, which was signal-forms-only — and the two cannot be combined, because `FormField` resolves a value accessor before a custom control, so a CVA replaces the native path rather than adding to it. `disabled` is now the union of the input and what a bound control writes through `setDisabledState`, and blur reports through `registerOnTouched` — which is what the `touch` output existed for, so it goes. The spec gains a reactive-forms host alongside the signal-forms one: value both ways, disable(), and touched, asserted against each API rather than against the interface members. The signal-forms host is now the guard against the interop path regressing.
The circle was the one part of the radio with no hover state. It now takes form.field.hover.border.color, and it triggers from anywhere on the card rather than from the circle alone, which is the point of a radio drawn as a card — a wider hover target than p-radioButton's own. Verified with a real hover: the card under the cursor goes from the form field's border to primary, the rest stay at rest.
A radio card only knows itself, so a group of them had no keyboard beyond Tab. The group adds what was missing: arrows move through the radios and wrap around, Home and End jump to the ends, disabled ones are skipped, and reaching a radio picks it — the way a native radio group behaves. Verified with real key presses in the browser, including that arrows and Home no longer scroll the page. It reads its radios from the DOM at the moment of the keypress, by `role="radio"` and `aria-disabled`, so it couples to nothing: any radio joins, and one disabled through a form control counts as disabled without having to announce it. No value, no form binding — the radios are already bound. It stops short of the ARIA pattern's single tab stop on purpose. A first pass did that with a member token and a roving tabindex, and the cost showed up immediately in the wrong place: the group read its members' state before Angular had set their inputs, which forced `option` to stop being a required input. The group's needs do not belong in the radio's API for a Tab stop nobody asked about, so Tab still visits each card and the card is untouched.
Every rule now sits in the one `form()` call instead of behind three `apply(path.x, xFormSchema(…))` indirections: reading the shell tells you what the form is, including the goal's length rule, the scheduling date bounds, the per-row weight ranges and the cross-field sum-to-100. Thirty lines, and no file to open to find out what a slice constrains. `WEIGHTS_TOTAL_ERROR_KIND` moves to `shared/constants` along the way: the shell raises it now and the variants card reads it, so a card was the wrong home. The three card specs restate the rules of the slice they mount on, which is the cost of this: they used to import the shell's schema, so their rules were the shell's by construction and now they are a copy. Each declares only what its assertions need, and the shell's own spec is what covers the real wiring. Single-field cards take `[field]`, since naming it after the slice said nothing the call site did not already say: `[trafficAllocationField]` and `[weights]` become `[field]`, joining goal and scheduling. Details keeps `[nameField]` and `[descriptionField]` — it takes two, so there the names carry their weight.
…ing a control The custom control is gone. The card is now a `<label>` around a `p-card` holding a `p-radioButton` bound with `[formField]`, which deletes everything we were maintaining in its place: the `ControlValueAccessor`, the value/option comparison, the click and keydown handlers, `role="radio"` with its `aria-checked` / `aria-disabled` / `tabindex` bookkeeping, and the hand-drawn circle. The earlier NG0201 finding was about a *bare* radio; bound with `[formField]` it works, so none of that was ever necessary. What replaces each piece is the browser's: the whole card is clickable because that is what a label does, the radio carries its own semantics, focus ring and hover, and radios bound to one field share the `name` the interop derives from the field's path — so `dot-radio-group` goes too, and its arrow keys with it. Verified in the browser: a real ArrowRight moves focus and selection between two cards, and the name is `ng.form0.goal.type` on all four. The one loss is Home/End, which native radios do not implement and our group did. The Goal card keeps its cascade through `(picked)`: the radio has written the type by the time it fires, so the handler only adds the proposed name and the fresh condition. Reacting to the pick rather than to the type changing is still what keeps a loaded goal's condition intact — and the debug console.logs that were sitting in that method are gone with it. The card's hover cue is the content hover background, not the form field's hover border: that token is `primary` in this theme, so a hovered card was rendering indistinguishable from the picked one. Both measured at #4e65f1.
`dot-experiments-select-page-dialog` is gone — 1288 lines of component, template, model and spec — and the Page card now opens `DotBrowserSelectorComponent`, the same dialog the file fields and the block editor open, asked for pages only. It answers with the chosen contentlet, and `identifier`, `title` and `url` are all the experiment needed. What made ours look necessary was a rule that does not exist: it greyed out every page already hosting a non-archived experiment. A page can host as many experiments as you like — `ExperimentsAPIImpl.save` puts no page-based restriction on creating one at all. What cannot happen is two *running* at once, which `start` rejects (or, for a scheduled one, an overlapping window). That is enforced where it belongs, at the transition, so blocking the pick was blocking a legal action. Two small changes make the shared browser fit a page picker, both defaulting to today's behaviour for its other two consumers: - The upload button is dropped when the browser lists neither files nor assets. A page cannot be uploaded, so the button was an empty promise. - The folder tree starts on the folder the browser was opened on rather than always on System Host, which is what made it open on an empty System Host listing instead of the current site. `DotPagesBrowserService.getFolderChildren` goes with the dialog that used it; the store keeps `searchPages` for the page prefill and `getPageLockState` for the lock. Eighteen dialog-only i18n keys go too — only the header survives, which the caller still sets.
…ligned Preview / Edit Content and Split Evenly were outlined buttons competing with the weight input for attention, and Split Evenly sat wherever its column started. Both are now text buttons — no border, no fill — pinned to the right edge of their column, so the three rows end on the same line and the only bordered control in the area is the one you type into.
…thing A list that fits in the smallest page size had a working rows-per-page select above it: picking 25 or 50 rendered the same single page either way. PrimeNG only disables that select at zero records, so it stayed live on four experiments. It is now offered only when the total exceeds the smallest option, through `rowsPerPageOptions` — PrimeNG renders it only when that is set, and it already disables the page arrows on a single page, so the whole bar goes inert together and reads "Page 1" with nothing to press. Verified in the browser on a four-experiment list. Dropped rather than greyed out on purpose: keeping it visible and disabled means CSS that a keyboard can walk straight past, since the select is a component and not a native control. This is the framework's own way of saying there is nothing to choose.
The weights only ever travel as a set adding up to 100 — `TrafficProportion` rejects anything else on construction — so leaving the arithmetic to the user meant typing 20 and then working out that the rest is 80, or with three variants, which two numbers add up to the remainder. The row just committed now decides its own share and the rest is spread over the others in the proportion they already had: 20 on a two-row split gives the other 80, and 50/30/20 with 20 committed on the first becomes 20/48/32. The rounding drift lands on the largest adjusted row, so three rows still add up to exactly 100. On commit — blur or Enter — and not on every keystroke, which is what separates this from a UI that fights back: typing 2 on the way to 25 would otherwise send the other rows to 98 and back. Typing alone changes nothing but its own row, and the amber total warning stays for what a commit cannot produce: an experiment arriving from the server at anything but 100. Three cases are deliberately left alone, since there is nothing to spread from: a cleared row, a value the range rules already reject, and a total that adds up already. Rows holding nothing between them share the remainder evenly, having no proportions to keep. Verified in the browser: 20/80 typed to 30 shows 30/80 while typing and settles at 30/70 on blur.
… set Spreading the remainder over every other row undid decisions as they were made: setting 15 on Original and then 50 on a variant moved the 15, which is the one number the user had just been explicit about. The remainder now goes to the rows they have not set. With 15 and then 50 committed, the third row takes the 35 that is left and the 15 stays put. Only when every other row has been set does one have to move, and then it is the one set longest ago — and only as far as the arithmetic demands, widening to the next oldest if that one cannot absorb it alone. Split Evenly forgets every decision, since it overrides every weight: after a split, the rows are all fair game again. Verified in the browser on a three-variant experiment: 27/15/58, commit 15 on Original for 15/17/68 (the two untouched rows share the 85 in proportion), then commit 50 for 15/50/35 — with the 15 untouched.
It appeared and disappeared in a single frame, which is most noticeable exactly when it happens most — while a split is being completed and the total crosses 100. It now enters over 150ms, settling from 2px up so it reads as the strip taking its place, and leaves over 120ms. Same short duration family as the screen's own transition and the content-drive toolbar, through the same `animate.enter` / `animate.leave` mechanism, and dropped entirely under `prefers-reduced-motion`.
… beside it The row already stretches its children and the design asks for exactly that (`align-items: stretch`), but PrimeNG's button keeps its natural height one element in, so it sat 8px short and read as floating above the field. `h-full` on it lets it fill the row. Measured both states: 48px against 48px with a page selected, 41px against 41px while none is, top and bottom flush in each.
`setName(id, name)` was `patch(id, { name })` under another name: same endpoint,
same verb, same mapping. It was added here to complete the legacy family of
single-key setters (`setGoal`, `setDescription`, `setScheduling`, ...) that the
Configure screen exists to replace with one accumulated multi-key call, and
nothing ever called it.
… why it overlaps The folder shapes went with the dialog that used them: `getFolderChildren` was removed when the Page card moved to the shared site browser, but `DotPageBrowserFolder`, `DotPageBrowserFolderChildren` and `DotPageBrowserFolderParams` stayed behind, exported from the barrel and referenced by nothing. Both docblocks still advertised folder navigation as a capability of a service that has two methods, neither of which browses folders. `DotPageSelectorService` already calls the same two endpoints, so the overlap is real and worth explaining rather than leaving for the next reader to find: a library cannot depend on an app, that service answers with the label/value shape an autocomplete needs rather than table rows, and it knows nothing about a page's lock state. Pointers now sit on both sides.
…e old screen does The display/edit swap was hand-rolled here with an `@if` and an `$isEditing` signal, while the old screen builds the same affordance on PrimeNG's `p-inplace` — which the theme already styles (`dotcms-theme/components/_inplace.scss`) and which gives the read state the `role="button"` and keydown handling the `@if` had no equivalent for. What was worth not reusing is the old `dot-experiments-inplace-edit-text` wrapper, which is ReactiveForms-based; the editor stays on signal forms. The docblock claimed that as the reason for hand-rolling the swap too, which it never was. `p-inplace` now owns the swap through `pInplaceDisplay`/`pInplaceContent`, Save and Cancel close it with `deactivate()`, and `onActivate` reseeds the draft from the persisted name so the editor opens the same way whichever affordance opened it. The sixteen existing tests pass untouched.
… fixed Five review findings on the Configure store and its cards. `validationErrors` was a snapshot frozen by the Start press, and only the Details card re-checked its own field against the live value — so seven of the eight rules stayed red after the user fixed them. The state now latches the *reveal* (`validationRevealed`) and the errors derive from the form, which drops the per-card re-checks and makes every rule clear itself. The scroll-to-first-error moves off the error list onto the press that caused it, or it would jump the screen on every keystroke that changed a rule. Autosave could lose an edit outright: `withoutSentKeys` settled a pending key by *name*, and `switchMap` only cancels a flight when the next debounce emits, so an edit made while the response was travelling was dropped with nothing left to resend it. Type, pause, keep typing, stop — and the server kept the older value while the form still showed the newer one. Keys now settle only while their pending value is still the one that went out. `enterExisting` reset the pending diff but not the validation state, so a URL pasted for another experiment carried the previous one's errors over. It now drops everything, as `enterNew` already did. A rejected page lookup was reported as "page not found" and never reached `DotHttpErrorManagerService`: a backend outage told the user the page did not exist and left ops with no signal. It gets its own event, its own copy, and the shared error manager. The variants counter set a static `aria-label` over its own text, so a screen reader heard "Variants used" and never the numbers. Two displaced JSDoc blocks move onto what they describe, and the Details card no longer claims a `required` rule the form never declares.
A PATCH against a local backend answers in a handful of milliseconds, so the indeterminate bar appeared and vanished inside a frame or two: the eye caught that something blue flickered under the header without ever resolving it into "saved", which reads as a glitch rather than as feedback. Once shown the bar now stays for at least `MIN_PROGRESS_BAR_VISIBLE_MS`, timed from when it appeared, so a fast save still gets a legible beat of it and a slow one keeps it up for as long as it takes. A save starting while a hide is pending cancels that hide and leaves the bar where it is, so back-to-back saves read as one continuous bar instead of a blink between them. The store is untouched: `$isSaving` still reports exactly when a request is on the wire, and how long that is worth showing stays with the screen that draws it. Its docblock also picks up a correction — `$isSaving` covers every request the screen makes, not just the autosave.
…tered `validateConfigure` read the goal and the weights off the persisted experiment only, which on `/experiments/new` is still `null`: a goal the user had just filled in went on being counted as missing until picking a page created the draft. Pressing Start on an empty form and working down the list — which is what the reveal is for — left the Goal card contradicting itself. The values were never lost, just unread. `formEdited` merges every key into the pending diff whether or not an experiment exists, and `applyPatchToExperiment` has nothing to apply an edit to before the POST answers, so the goal lives only there. The rules now read the diff first and fall back to the experiment, which is the same shape `draftName` and `selectedPage` already had for the two fields that could be filled in before creation. `minVariants` stays unsatisfiable until the draft exists, and that is not this: adding a variant needs an experiment id, so the card keeps `Add new variant` disabled until the POST answers.
…query
`?pageId=` is whatever the address bar carries, and both page lookups concatenate
it straight into a query string: `+identifier:${pageId}` in the prefill search
and in `getPageLockState`. A value carrying spaces or Lucene operators widens the
query instead of naming an id, so a crafted URL could prefill the Page card with
another contentlet, or answer with another contentlet's lock state. The endpoints
are permission-scoped and read-only, so this is query correctness rather than
privilege escalation — but it is a user-input entry point this screen opened.
`isDotIdentifier` states the shape once, next to the other shared helpers.
Nothing outside a 36-character UUID can name a page, so both call sites answer as
they would for a page that is not there — the prefill reports not-found, the lock
lookup reports unlocked — without spending a request on it.
The fixtures said `page-1`, which is not an identifier any dotCMS install would
produce; a fake that shape is why nothing here looked risky. They are UUIDs now,
which is also what makes the two guard tests meaningful.
Reported by claude[bot] on the PR.
`$canClearSchedule` keyed off the start date, but an end date alone is a schedule
the backend keeps: `toRange` sends `{ startDate: null, endDate }`, which reads as
"start when Start is pressed, stop then". Setting only an end date therefore hid
the control for a schedule that was really there — and its own docblock already
said "before a date is set", not "before a start date is set".
Either date now counts. `clearSchedule` needed nothing: it always dropped both.
Not a dead end before this — each picker carries `showClear`, so the end date
could still be cleared on its own. What was missing was the affordance that drops
the schedule as one choice, in a state where there was a schedule to drop.
Reported by Copilot on the PR.
The Results screen needs the Chart.js options, the interactive legend plugin, the p-chart wrapper, the headless details table and the winner/promoted/chart-data helpers. All of them live under old/, which is frozen and slated for deletion, so they are relocated to shared/ and old/ is repointed at the new paths rather than duplicating them: one source of truth, and old/ stays deletable. Only import lines change under old/, so its behaviour is untouched and its specs still pass unmodified. Three importers the plan had not accounted for turned up in the Configure legacy screen and needed the same repoint. getConversionRate, getConversionRateRage, getBayesianVariantResult and getProbabilityToBeBest stay where they are: they are not needed outside old/, and the dependency direction has to remain old/ -> shared/, never the reverse.
State is {experiment, results, status, refreshing, lastRefreshFailed}, keyed by
experimentId alone, so the screen is page-independent the way the old reports
store already was.
The load runs sequentially rather than as the old screen's forkJoin, because the
second call depends on the first: DRAFT and SCHEDULED settle on getById alone and
never reach getResults, which is uncached and costs two analytics round-trips plus
a Monte Carlo run. The branch reads the experiment's status, not whether results
happen to be null.
A failed refresh leaves results and status untouched, so the last good report
stays on screen and only a flag says the refresh failed. A first load that loses
only its report keeps the experiment and reports inline; blanking there would
regress against the screen this one replaces.
Lift vs Original is computed here since no field carries it. The baseline is
resolved by key and isControl by name, never by row position, so the order rows
arrive in cannot change the arithmetic.
The refresh flag is raised only for the statuses the handler will actually serve;
raising it for the others would spin forever with no request in flight.
Wires :experimentId/results, reachable on every status including DRAFT, and hangs dotAnalyticsHealthCheckResolver off this route only so a broken analytics app takes out the report and leaves the list reachable. That resolver is a plain ResolveFn: it reports rather than redirects, so the screen reads the resolved status and renders the misconfiguration state in place of the report. The edit-page-coupled AnalyticsAppGuard is deliberately not reused. DotExperimentsService is provided on the route rather than the component. The resolver injects it and runs in the route injector, before the screen exists, so providing it on the screen alone would still throw NG0201 on activation. configureCommandsOf moves to the list util: the list's row action and the Results header's Configuration button are two ways to the same URL, and a URL spelled out twice is a URL that can drift.
Shell, header, stat strip, Daily/Bayesian tabs and summary table, per the approved prototype. Both charts reuse the relocated Chart.js options and legend plugin; the prototype's inline SVG is not reimplemented and the Bayesian posterior comes entirely from the backend. The two charts render in @if/@else so exactly one canvas is ever mounted: the legend plugin resolves its container by walking up from the canvas and calling querySelector, which from a shared ancestor returns the first match in document order, so two mounted charts would silently claim each other's legend. The stat strip is presentational and takes the winner legend from the backend's suggested winner, never from a locally computed highest rate, so "no winner yet" renders instead of a false Leading Variant claim. Goal and period come off the experiment, so they still read when the report does not. The session gate is experiment-wide: below ten total sessions the whole summary is one empty state, with no per-row filtering above it. Promote asks first from both entry points -- the strip's inline shortcut and the table's per-row button -- since promoting a RUNNING experiment ends it server-side in the same call. The table hosts its own dialog so the shell and the table never share a key, which would open both at once.
Leads the row's action cell, with Configure staying the first kebab entry, as the design lays it out. Ungated on purpose, unlike every kebab entry: AllowedActionsByExperimentStatus clears RUNNING and ENDED only, but the report renders a waiting state of its own for an experiment with nothing counted yet, so the row offers it on every status. Also drops the module-local configureCommandsOf for the shared helper.
Descriptive spec for the standalone Experiments portlet as a whole, across all its screens, capturing the decisions and invariants that until now lived only in code and commit messages. Screens 1-3 are already built, so requirements carry a [BUILT] or [PENDING] marker and the spec states plainly that the constitution's TDD gate applies to the pending work and to future changes, not retroactively to code that already ships. The load-bearing invariants are quarantined in their own section, stated in implementation terms on purpose: the events-plugin store contract and its version constraint, why results are fetched lazily and never polled, why significance is the backend's call, why a lost report must not cost the whole screen, and why nothing in the new tree may import from old/. Refs #36987
oidacra
force-pushed
the
issue-36987-experiments-portlet-spec
branch
from
August 20, 2026 19:46
de81d1b to
5e78ef9
Compare
Review of the spec turned up fourteen places where it described software that does not exist, or claimed an invariant the code deliberately breaks. Every one was reproduced against the code before being written down. Template conformance: - Status is Draft, as the template and both sibling specs use until PR 1 is approved. The descriptive-baseline caveat already lives in Read This First. - The issue is carried by a GitHub Issue field in the siblings' format, naming the parent epic, rather than by two bespoke fields. - User Story 5 is gone. Its three acceptance scenarios were already FR-074, FR-075 and FR-076 verbatim, and the story admitted it was a developer guarantee rather than a user journey. - SC-009 no longer counts twelve behaviors it never listed. It states the rule the parity walk must satisfy; producing the list is plan work, now recorded under Open Questions. Claims the code contradicts: - FR-006 permitted no departure from the shared status-to-actions map. Two exist and both are deliberate, so it now names them and forbids a third. Restore gets FR-021a: rendered for an archived experiment, permanently inert until the transition lands with #36988, gated by an inline status check because the shared map has no restore key and acquiring one would change what the legacy list reads. - FR-021 advertised restore as an offered action. It is inert. - FR-022 and FR-006 collided and an Assumption covered the mechanism. The collision is resolved in the spec: the new list does not consult the map for View Results, and the map's results key stays RUNNING and ENDED for the legacy list. Widening it is forbidden. - FR-075 said only import lines may change under the legacy subtree, while seventeen files and 973 lines left it. Relocating a component out of the subtree with its own spec is the intended outcome; the prohibitions are duplication and any new-to-legacy import, and neither is violated today. FR-075a permits duplication only where importing would mean touching legacy code, and requires it declared at the definition site -- as the one instance, the status-to-severity map, already is. - FR-076 constrained the legacy test count. It constrains legacy behavior. - AI-9 counted three dialog keys. Two exist: the Results shell owns one, Configure and the list share the other because they never co-mount, and the summary table raises on the shell's. The invariant is co-mounting, so sharing is the default. - Legacy Considerations said the page-editor entry point sits behind a flag that is off by default. The route mounts unconditionally and LOAD_FRONTEND_EXPERIMENTS is read nowhere; the gate is Screen 4 (#37005). - FR-062 read as accidental coupling to the chart. The threshold is one constant on purpose; MINIMUM_SESSIONS_TO_SHOW_CHART is the wrong name for what it now gates, and the rename is a tracked follow-up.
erickgonzalez
approved these changes
Aug 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A descriptive specification for the standalone Experiments portlet as a whole —
across the list, create/update and results screens — rather than for any one of
them. It captures the decisions and invariants that until now lived only in
docblocks and commit messages, and it is the baseline the portlet's remaining
screens are planned against.
Refs #36987. Covers #36989, #37003, #37004 as built, and #37005, #37006, #37007,
#37008 as pending.
Why it reads as documentation, not as a request
Most of this feature already ships. Every requirement therefore carries a
[BUILT]or
[PENDING]marker, and the spec states in its opening section that theconstitution's TDD gate (Principle V) applies to the pending work and to future
changes — not retroactively to code that is already merged or in review. It
does not reconstruct a Red phase that never happened.
What it records
that are easy to lose: keeping the portlet usable when analytics is broken, and
keeping the legacy tree deletable in one commit.
health / coexistence.
constraints in implementation terms on purpose — the events-plugin store
contract and its
@ngrx/signalsversion constraint, why results are fetchedlazily and never polled, why significance is the backend's call rather than a
conversion-rate comparison, why a lost report must not cost the whole screen,
and why nothing in the new tree may import from
old/. Paraphrasing these intotechnology-neutral language would have destroyed the constraint they impose on
the remaining screens.
not reopened.
issues, PRs, source files and commits.
Follow-ups this surfaced
Planning against the spec turned up three things worth fixing on the pending
issues before they are picked up:
site-wide/to the lib root". Nosite-wide/directorywas ever created — the new code shipped at
src/lib/with the legacy quarantinedin
src/lib/old/. The migration is a deletion, not a promotion.shared/tree". There are two:old/shared/goes,and
src/lib/shared/must stay — it holds the charts, legend plugin and detailstable the Results screen renders from.
goal filter with counts and a repeatable
goalURL param, and Add pagination, sorting, filtering and permission checks to the Experiments list endpoint #36823's contracthas neither.
Scope of the diff
One new file,
specs/36987-experiments-portlet/spec.md. The plan, research andchecklist artifacts stay local, per the spec-folder commit policy in
.specify/CUSTOMIZATIONS.md.Verification
Documentation only — no source, no build config, no tests touched. Prettier is
scoped to
core-web/and does not coverspecs/at the repo root, so there is noformatting step that applies. Nothing to lint or run.
This PR fixes: #36987