docs(experiments): specification for the Experiments portlet - #37144
docs(experiments): specification for the Experiments portlet#37144oidacra wants to merge 2 commits into
Conversation
|
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 |
5305f6a to
64ee5b7
Compare
64ee5b7 to
de81d1b
Compare
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
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.
2 participants
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