fix(control-panel): read the Experiment back through one refresh after every write (SPL-268)

[isuttell]

Summary

The Experiment detail is served by a React Query cache (experimentDetailQuery, subscribed in lib/experiment-detail-route.ts:16). router.invalidate() re-runs the route loaders but does not touch that cache, so a surface that uses only it shows the operator the state before their own write — the disguised default ADR-0036 forbids. useExperimentDetailRefresh exists precisely to clear both, and its own doc comment says so.

SPL-264 is what that gap costs when a rendered field reads from the stale cache: a started Run was invisible in Run history until a reload. That one was fixed by #248. This PR closes the two remaining surfaces with the same gap, plus the mirrored contradiction on the Metrics page.

Changes

• experiment-measurement-form.tsx and experiment-metadata-form.tsx route their post-write read-back through useExperimentDetailRefresh instead of router.invalidate() alone. No third mechanism added — the point is that one already existed and these two bypassed it.
• New source invariant lib/experiment-detail-refresh-invariant.test.ts: it names every caller of updateControlPanelExperiment / stageAndStartControlPanelExperimentRun and fails if one skips the hook or reaches for router.invalidate() on its own. Same shape as the existing app-scoped-link-invariant.test.ts. Because the bug is latent, this invariant is the entire regression story, so review round 1 hardened it — see Review round: the guard was defeatable below.
• metrics-page.tsx re-reads instead of patching locally. It had been splicing the write's own response into visibleMetrics — exactly the local patching use-experiment-detail-refresh cites ADR-0023 to forbid. router.invalidate() is the whole read-back there because Metrics are plain route-loader data with no query cache in front of them. The local mirror, its resync useEffect, and the now-dead upsertMetric/removeMetric exports (and their unit test) are gone.

Risk: LOW

• Areas touched: Control Panel Experiment Setup forms, Metrics page. No Control Plane, contract, or Worker change.
• Security: none.
• Performance: one extra Experiment detail read per Setup-form save. On the Metrics page, reread (metrics-page.tsx:28-30) re-runs the whole route loader, which is loadScopedSession and loadControlPanelMetrics — two server calls per Metric write, not one, and a deleted row lingers on screen for that round trip instead of vanishing on click. Still the right tradeoff: the alternative is showing the operator a row the Panel never read back.
• Breaking: none.

Test plan

RED → GREEN on the new invariant

RED, with only the two form files reverted to dfc2c902 and the new test in place:

FAIL src/lib/experiment-detail-refresh-invariant.test.ts > routes every one of them through useExperimentDetailRefresh
AssertionError: expected [ …(2) ] to deeply equal []
- []
+ [
+   "components/experiment-measurement-form.tsx",
+   "components/experiment-metadata-form.tsx",
+ ]

FAIL src/lib/experiment-detail-refresh-invariant.test.ts > leaves no second read-back mechanism beside it
AssertionError: expected [ …(2) ] to deeply equal []
- []
+ [
+   "components/experiment-measurement-form.tsx",
+   "components/experiment-metadata-form.tsx",
+ ]

 Test Files  1 failed (1)
      Tests  2 failed | 1 passed (3)

GREEN, with the fix applied:

 Test Files  1 passed (1)
      Tests  3 passed (3)

That reverting the fix is exactly what flips it is the mutation proof: the test fails without the change and passes with it, and it names the offending files rather than asserting a generality.

Review round: the guard was defeatable

Round 1 was right that the caller predicate was too loose to carry the proof. It required ${fn}({, so it only saw call sites that inline their object argument. It now matches the bare identifier over comment-stripped source, and test 2 requires the refresh to be bound and awaited rather than merely named.

Each escape shape was run through both predicates. INVISIBLE means the file was not even collected as a caller, so all three tests passed while the surface did no read-back:

Escape shape	old name({	new bare id
hoisted payload — const input = {…}; updateControlPanelExperiment(input)	INVISIBLE	DETECTED, Tests 2 failed | 1 passed (3)
aliased import — import { updateControlPanelExperiment as save }	INVISIBLE	DETECTED, same
wrapper module — call moved behind a local helper	INVISIBLE	DETECTED, same (see caveat)
spaced paren — updateControlPanelExperiment ({…})	INVISIBLE	DETECTED, same

Test 2 got its own two probes, both of which the old test 2 passed:

• probe-imports-only.tsx — names useExperimentDetailRefresh in an import it never calls.
• probe-bound-unawaited.tsx — binds const refresh = useExperimentDetailRefresh(…) and calls refresh() without awaiting.

Both now fail on routes every one of them through an awaited useExperimentDetailRefresh.

Two disclosures rather than quiet narrowing:

• The census in the review was incomplete, and a bare identifier would have produced a new false red. components/experiment-draft-run-step.tsx names stageAndStartControlPanelExperimentRun in a doc comment saying it does not call one directly, and correctly delegates to useExperimentRunStart. Resolved with a code() helper that strips comments before matching, not by trimming the table or adding an exception list. It is deliberately conservative — a trailing // name after code survives, which is a false red, never a false green.
• The wrapper shape is caught at the wrapper module, not at the downstream consumer. The wrapper becomes an unlisted caller, which fails test 1's pinned list, so the change cannot ship silently. But the coverage there is by-forced-review rather than direct: the guard makes someone look, it does not itself prove the consumer refreshes.

The guard's header comment now carries the reviewer's note on why "latent" is not "unobservable": the router builds a bare new QueryClient() (router.tsx:8), so a newly mounted useSuspenseQuery subscriber renders the cached value first and refetches behind it, and any component seeding useState from that first render keeps the stale value permanently. A client-side <Link> between the setup tab and the draft wizard, or any read-only render of one of these fields, makes it assertable.

Why there is no rendered e2e assertion for these two forms

Stated plainly because it matters for how much this PR proves. Neither form's staleness is observable in the rendered Panel today, so an e2e assertion for it would pass with and without the fix, which is worthless.

conversionWindowMs, description, owner, and tags are read by no mounted surface other than the forms that own them, and those seed local state on mount. Every path that remounts them currently performs a fresh read for an unrelated reason: Run start refreshes (#248), a reload refetches, and a client-side nav remounts the useSuspenseQuery subscriber, which refetches at staleTime: 0. The one reachable window — save, then open a surface that reads the cached detail while the subscriber stays mounted — renders stale data only until the background refetch lands, which is a race, not an assertion.

So this is a latent correctness gap, closed at the level where it is actually decidable: the source invariant. experiments-setup.spec.ts remains the rendered half of the proof for the field that is re-read (Run history), and it stays green.

Runs

Control Panel e2e — fleet lock held, acquireMachineLock("local-e2e-fleet"), --workers=1, SPLITCH_PLATFORM_TARGET=pr-ci, workspace deps built first:

[3/3] ../../e2e/control-panel/metrics.spec.ts:30:3 › App-level Metrics › round-trips every aggregation and surfaces Worker validation
  3 passed (19.1s)
playwright exit code: 0

metrics.spec.ts is a real regression guard on the re-read: it creates, renames, and deletes Metrics and asserts each row appears and disappears with no reload, which now happens through the route loader rather than a local splice.

No e2e re-run for the review round: it changed a test predicate only, no runtime code.

One plain pnpm verify:ci, no TURBO_FORCE, on the review-round tree, exit code 0:

 Tasks:    85 successful, 85 total
Cached:    79 cached, 85 total
  Time:    10.384s

The six cache misses are exactly the tasks this diff can affect: //:format:check, //:knip, and @splitch/control-panel:lint, :typecheck, :build, :test — all cache miss, executing. No task belonging to the modified package replayed. (The earlier run on the original tree was 85 successful / 76 cached, exit 0, with the same control-panel tasks executing.)

Issue: SPL-268

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. To trigger a review, include @coderabbitai review in the PR description. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a261d1e5-cc41-412f-a467-8225166a97e8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}