commit 7c2f2b705aceeeedbedc598d680b036a60b474ea
parent 697378098fbc49c58d00b98952baa263d5aa72f5
Author: I Mean I'm Just Saying <imeanimjustsaying@kiwifarms.st>
Date: Tue, 8 Sep 2026 16:53:06 -0400
plans: a keyed move keeps its state, so name what actually unmounted the panel
Five corrections from the review of `a6e6d46`. No code changes.
- The React rule was stated wrong in all three files. "A moved child is
unmounted and mounted again" is FALSE — React preserves state across a KEYED
move. Static JSX fragment children compile to one array, so
reconcileChildrenArray runs, its index fast-loop breaks at 0 (p vs dl), and
the map pass finds ul/p at index 1 where the UNKEYED RunOne was: a positional
TYPE MISMATCH is what destroyed it. A key alone would have matched the old
fiber at its new index; a fixed position alone would never have reached the
map pass. Each was sufficient — the commit ships both, belt and brace.
- deflake-e2e.md quoted one shape for both bodies. DiarizationBody was
[p, RunOne] vs [dl, p, RunOne] (index 1 → 2); AttributionBody was [p, RunOne]
vs [dl, ul, p, RunOne] (1 → 3). Both are stated now, as a table.
- "H1 held" was too generous. H1 as written — an early SSE teardown, fixed by
polling the log file until terminal — was killed by the trace too: zero
stream.cancel in 240 instrumented tests. Only its DISPOSITION held (server
innocent, failure client-side); the cause is a sixth mechanism outside the
table, and the fix H1 prescribed was not shipped and would not have worked.
- attribution-batch-hang.md had no outcome. It has an "## Outcome" section now.
- The durable rule in FACTS.md was too narrow. "Must not change position" misses
the same effect by a different mechanism: a parent that stops rendering the
panel at all. Three pre-existing instances, none under a spec, so none flakes,
all three known and unfixed and out of scope here — VideoPanel.tsx:1060
(SourceVideoSection early-returns the persisted view once savedVideo is
truthy, destroying the "Persist source video" log at :1133) and the two
banners at :1494 and :1557, dropped by their parent at :237-255 once the run
clears the condition that drew them. The rule is now: a component carrying a
StreamActionLog must not be UNMOUNTED when the record its run produced lands.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Diffstat:
4 files changed, 103 insertions(+), 45 deletions(-)
diff --git a/plans/FACTS.md b/plans/FACTS.md
@@ -3526,23 +3526,40 @@ entry calls `missingInput`; its `eligible`/`present` are the playlist and
### A run log lives in the panel's React state, so the panel must not move (`fae99f1`)
-`StreamActionLog` keeps the streamed log in `useState` and calls `router.refresh()` itself the
-moment a run ends (`common/components/StreamActionLog.tsx:156-163`). That refresh re-renders
-the SERVER component that draws the panel — with the sidecar the run just wrote now on disk.
-So any panel that changes the SHAPE of its child list when its record appears destroys the log
-it was showing: React reconciles a fragment's children by POSITION, a moved child is unmounted
-and mounted again, and with `log` empty and `running` false `StreamActionLog` renders no
-`<pre role="log">` at all — the element the operator (and the spec) is reading is not stale,
-it is gone. `SpeakerBodies.tsx` had exactly that: `[empty state, RunOne]` before a record,
-`[dl, ul, p, RunOne]` after. Both bodies are one two-child list now, the record body a sibling
-of the button, with `<RunOne key="run-one" …>` last in both branches
-(`editor/app/channels/[slug]/videos/[id]/components/SpeakerBodies.tsx:86-99,166-179`) — the
-position is the fix, the key is the brace behind it. **This is the shape to copy for any new
-panel that carries a `StreamActionLog`** — `DigestBody.tsx:71-131` was already written that
-way (one return, one ternary, the log at a fixed index), which is why only the speaker panels
-flaked — and it is why `attribution.spec.ts:346` flaked 1-2 runs in 10 for a batch that had
-finished in 65 ms with every line written. The full trace and the four hypotheses it killed
-are in `plans/deflake-e2e.md`'s "4, as shipped".
+**THE RULE: a component carrying a `StreamActionLog` must not be UNMOUNTED when the record its
+own run produced lands.** `StreamActionLog` keeps the streamed log in `useState` and calls
+`router.refresh()` itself the moment a run ends (`common/components/StreamActionLog.tsx:156-163`).
+That refresh re-renders the SERVER component that draws the panel — with the sidecar the run
+just wrote now on disk. Unmount it in that render and the log is not stale, it is GONE: with
+`log` empty and `running` false the component renders no `<pre role="log">` at all. Two ways
+that happens, and the rule has to cover both.
+
+**Moved unkeyed across a differing sibling shape.** `SpeakerBodies.tsx` returned
+`[empty state, RunOne]` before a record and `[dl, (ul,) p, RunOne]` after. Static JSX fragment
+children compile to ONE array, so `reconcileChildrenArray` runs: the index fast-loop breaks at
+0 (`p` vs `dl`), and the map pass then looks up index 1 and finds `ul`/`p` where `RunOne` used
+to be — **a positional TYPE MISMATCH is what destroyed it, not the move itself**. React
+preserves state across a keyed move: a `key` alone would have matched the old `RunOne` fiber at
+its new index with state intact, and a fixed position alone would never have reached the map
+pass. Either was sufficient; the fix ships both — the record body is a sibling of the button
+and `<RunOne key="run-one" …>` is last in both branches
+(`editor/app/channels/[slug]/videos/[id]/components/SpeakerBodies.tsx:86-99,166-179`).
+`DigestBody.tsx:71-131` was already written that way (one return, one ternary, the log at a
+fixed index); only the speaker panels were under a spec that could see it, which is why only
+they flaked — `attribution.spec.ts:346`, 1-2 runs in 10, for a batch that had finished in 65 ms
+with every line written.
+
+**Dropped by a parent branch swap** — same effect, different mechanism, and `VideoPanel.tsx`
+has three instances of it, all pre-existing, none under a spec, so none of them flakes and all
+three are KNOWN AND UNFIXED (out of scope for `fae99f1`; the operator decides whether they are
+worth a commit). `SourceVideoSection` early-returns the persisted view once `savedVideo` is
+truthy (`:1060`), so the *Persist source video* log (`:1133`) is destroyed by the
+`router.refresh()` its own run fires. `IncompleteTranscriptBanner` (`:1494`) and
+`ShortAudioBanner` (`:1557`) are the same shape from the other side: the parent stops rendering
+them (`:237-255`) once the run clears the condition that drew them.
+
+The full trace, and why five of the plan's hypotheses — H1's mechanism included — were all
+killed by it, is in `plans/deflake-e2e.md`'s "4, as shipped".
### The `editor/content` symlink still blocks e2e in the primary checkout
diff --git a/plans/STATE.md b/plans/STATE.md
@@ -106,21 +106,31 @@ Error: locator.getAttribute: Test timeout of 120000ms exceeded.
```
The job finished in **65 ms**, wrote all five lines including "1 done", enqueued every one of
-them with `closed=false`, and finalized `done`. The log panel was **gone** — not stale, not
-short, absent: the failure snapshot shows the record body and the Run button and no
-`<pre role="log">` at all. `AttributionBody` returned two fragments of different SHAPES
-(`[empty, RunOne]` with nothing on disk, `[dl, ul, p, RunOne]` with a record), React reconciles
-a fragment's children by POSITION, so the record landing moved `<RunOne>` down the list — and a
-moved child is unmounted and re-mounted, taking `StreamActionLog`'s `log` state with it. The
+them with `closed=false`, and finalized `done`. **All five hypotheses the plan ranked are dead,
+H1's mechanism included** — no `stream.cancel` fired in 240 instrumented tests, so the early
+SSE teardown H1 described (and the log-file polling it prescribed) was never happening; all
+that held of H1 was its disposition, server innocent and failure client-side. The real cause is
+a sixth mechanism outside the table: the log panel was **gone** — not stale, not short, absent.
+The failure snapshot shows the record body and the Run button and no `<pre role="log">` at all.
+`AttributionBody` returned two fragments of different SHAPES (`[p, RunOne]` with nothing on
+disk, `[dl, ul, p, RunOne]` with a record; `DiarizationBody` the same at `[dl, p, RunOne]`), so
+`reconcileChildrenArray`'s index pass broke at 0 and found `ul`/`p` where the UNKEYED `RunOne`
+had been — a positional type mismatch, which is what unmounts a child and takes
+`StreamActionLog`'s `log` state with it. React preserves state across a KEYED move, so a key
+alone or a fixed position alone would each have sufficed; the fix ships both. The
refresh that lands the record is the one `StreamActionLog` fires itself when the run ends, so
the panel is wiped at the instant the summary line reaches it, and the spec could only pass in
the ~200 ms window before that. **The earlier note here — "the job never reaches a terminal
state", "the hang is in the BATCH" — was the artifact, and so was "the log FILE has no more
than the panel": the file had everything, and no reconcile could have helped a component whose
-state had been destroyed.** Do not trust either claim again. Both bodies are one two-child list
-now, RunOne last and keyed; the spec asserts the log still says "1 done" AFTER the pill reads
-"current", which is red on every run at `fea2996` (5/5) and green at the fix.
-`attribution.spec.ts --repeat-each 20`: **20/20**.
+state had been destroyed** — nor would the plan's own H1 fix have helped, for the same reason.
+Do not trust either claim again. Both bodies are one two-child list now, RunOne last and keyed;
+the spec asserts the log still says "1 done" AFTER the pill reads "current", which is red on
+every run at `fea2996` (5/5) and green at the fix. `attribution.spec.ts --repeat-each 20`:
+**20/20**. The durable rule — **a component carrying a `StreamActionLog` must not be UNMOUNTED
+when the record its run produced lands**, whether by an unkeyed move or by a parent branch
+swap — is in `FACTS.md`, with three pre-existing unfixed instances in `VideoPanel.tsx` that no
+spec covers.
**The last full suite: 514 passed, 1 failed of 515, 25.1 min** from a worktree of `2133d94`.
The red was slice 1.5's own new band assertion and it was right — writing the two new
diff --git a/plans/attribution-batch-hang.md b/plans/attribution-batch-hang.md
@@ -166,3 +166,18 @@ no-boot-against-`transcripts/`, detached-e2e and DO-NOT-POLL (Monitor) instructi
report contract = commit shas, exact gate outputs, the decisive trace lines, which
hypothesis held and which were killed, every divergence. A second Opus agent reviews the
diff before the final full e2e.
+
+## Outcome (2026-09-08, `fae99f1`)
+
+The cause was none of H1–H5 but a sixth mechanism outside the table: the job finished normally
+in 65 ms with every line written and `done` recorded, and React then UNMOUNTED the log panel —
+`SpeakerBodies.tsx`'s two branches put the unkeyed `<RunOne>` at different child indices, so
+the record landing (on the `router.refresh()` `StreamActionLog` fires itself) hit a positional
+type mismatch and destroyed `StreamActionLog`'s state, `<pre role="log">` and all. H1's
+disposition held (server innocent, failure client-side) but its mechanism did not, and the
+log-file-polling fix prescribed for it was NOT shipped and would not have worked.
+
+Fixed by giving both bodies one two-child list with `RunOne` last and keyed; the spec now
+asserts the log still reads "1 done" after the freshness pill reads "current" (red 5/5 at
+`fea2996`, `--repeat-each 20` green at the fix). Full record, trace and the durable rule:
+[`deflake-e2e.md`](deflake-e2e.md)'s "### 4, as shipped" and `FACTS.md`.
diff --git a/plans/deflake-e2e.md b/plans/deflake-e2e.md
@@ -135,32 +135,48 @@ Error: locator.getAttribute: Test timeout of 120000ms exceeded.
```
The whole job took **65 ms**. Every line reached `onLog` with `closed=false`, the summary
-included, and the record finalized `done`. So of the five hypotheses the plan ranked, **H1
-held and H2-H5 are dead**: no `stream.cancel` fired in 240 instrumented tests (40 of this
-spec), `limit()` never read 0
-(`pool.wait-capacity target=1` throughout), `runOne`'s `finally` timestamps are all present,
-finalize ran, and no signal aborted.
-
-What the panel was doing is the answer, and it is not a stale log — the log ELEMENT was gone.
-`AttributionBody` (and `DiarizationBody`) returned two fragments of different SHAPES:
-`[empty state, RunOne]` with no record on disk, `[dl, ul, p, RunOne]` with one. React
-reconciles a fragment's children by POSITION, so the record appearing moved `<RunOne>` from
-index 1 to index 3 — and a moved child is unmounted and mounted again, taking
-`StreamActionLog`'s `log` state with it. With `log` empty and `running` false the component
-renders no `<pre role="log">` at all, which is exactly what the failure snapshot shows: the
-record body, the Run button, and no log. The refresh that lands the record is the one
-`StreamActionLog` fires itself at the end of a run (`:163`), so the panel is wiped at the
+included, and the record finalized `done`. **The trace killed all five hypotheses the plan
+ranked, H1's mechanism included.** H2: `limit()` never read 0 (`pool.wait-capacity target=1`
+throughout). H3: `runOne`'s `finally` timestamps are all present. H4: finalize ran. H5: no
+signal aborted. And H1 as WRITTEN — an early SSE teardown, to be fixed by polling the job's
+log file until terminal — is dead too: `stream.cancel` fired **zero** times in 240
+instrumented tests (40 of this spec) and every line enqueued `closed=false`. All that held of
+H1 was its DISPOSITION: the server is innocent and the failure is client-side. The real cause
+is a sixth mechanism, outside the plan's table, and **the fix the plan prescribed for H1 was
+not shipped and would not have worked** — see below.
+
+**The sixth mechanism: React unmounted the panel.** The log was not stale — the log ELEMENT
+was gone. Both bodies returned two fragments of different SHAPES, each with `RunOne` at a
+different index in the two branches:
+
+| body | no record on disk | with a record | RunOne moves |
+|---|---|---|---|
+| `DiarizationBody` | `[p, RunOne]` | `[dl, p, RunOne]` | index 1 → 2 |
+| `AttributionBody` | `[p, RunOne]` | `[dl, ul, p, RunOne]` | index 1 → 3 |
+
+Static JSX fragment children compile to one array, so `reconcileChildrenArray` runs: the index
+fast-loop breaks at 0 (`p` vs `dl`) and the map pass then finds `p`/`ul` at index 1 where
+`RunOne` used to be. **A positional TYPE MISMATCH on an UNKEYED child is what destroyed it** —
+not the move as such: React preserves state across a KEYED move, so a `key` alone would have
+matched the old fiber at its new index, and a fixed position alone would never have reached
+the map pass. Either was sufficient. With `log` empty and `running` false the remounted
+component renders no `<pre role="log">` at all, which is exactly what the failure snapshot
+shows: the record body, the Run button, and no log. The refresh that lands the record is the
+one `StreamActionLog` fires itself at the end of a run (`:163`), so the panel is wiped at the
instant the summary line reaches it, and the spec could only pass in the ~200 ms window
before the refresh landed.
**This retires two claims made here on 2026-09-08 and both were measurement artifacts**: "the
job never reaches a terminal state" (it reached it in 65 ms) and "the log FILE has no more
than the panel does" (the file had everything). It also explains why the two panel-side
-reconciles measured no change — no amount of reconciling helps a component React has just
-destroyed and rebuilt with empty state.
+reconciles measured no change, and why the plan's prescribed H1 fix would have measured no
+change either — no amount of reconciling helps a component React has just destroyed and
+rebuilt with empty state.
Fix: both bodies are ONE two-child list, the record body a sibling of the button, `RunOne`
-last and keyed in both branches. No DOM change, no server change, `StreamActionLog` untouched.
+last AND keyed in both branches — belt and brace, since each alone would have done. No DOM
+change, no server change, `StreamActionLog` untouched. The durable rule this generalises to
+(and three pre-existing, unfixed instances of it in `VideoPanel.tsx`) is in `plans/FACTS.md`.
Test: the spec asserts the panel STILL contains "1 done" AFTER the freshness pill reads
"current" — the pill is the proof the refresh landed, which makes it deterministic rather than
a second race. At `fea2996` that assertion is red **5/5** (`element(s) not found`); at
