Twinned from https://github.com/cfsh/ninefold/issues/475 by tools/beads/import_github.py's reconcile pass.
Observation from #453, worth capturing while it is fresh — the reviewer named it too.
Across four review passes on that PR, every finding after the first round was in the same seam, and the last two rounds found only defects introduced while fixing the previous round's:
| | defect | shape |
|---|---|---|
| D1 | flip back to split blanked the editor | ApplyLayout set visibility on a container that was never hidden, and returned before the code that owns card visibility ran |
| D2 | _flowFill on the wrong tree, or freed | ApplyLayout re-parents the roll; the fill panel was parented once |
| D6 | deep dive gave half its height to home's spacer | spacer visibility owned by ApplyLayout, screen visibility owned by ShowScreen |
| D7 | △ dove into the wrong thing | TriangleJump read focus *after* ShowScreen released it |
| D10 | △ from a non-cell ran the return branch | branch chosen by focus state rather than by screen state |
| D11 | flip with the fill panel up hid the roll | ApplyLayout rebuilds the tree without resetting page state |
| D13 | resize killed the walk in both layouts | the D11 fix landed above the early return, and ApplyLayout is also the resize handler |
| D14 | △ opened a pattern you did not pick | the D10 fallback re-derived state the player had already chosen |
Eight defects, one seam. The through-line is that three concerns are interleaved in two methods:
ApplyLayout, also wired to Root.SizeChangedShowScreen_jumpCell, _flowFill, the editing marker, the note cursor) — owned by nobody in particular, mutated by bothEvery fix so far has been "move this line to the other side of that guard", which is why each one had a decent chance of breaking a neighbour. The reviewer's framing was that ApplyLayout rebuilds the tree without resetting the page's transient state — that is true three separate times (D2, D11, D13) with three different pieces of state.
PageState owner for the transient set, with one Reset() that every rebuild path calls, instead of _jumpCell/_flowFill/editing-marker each being cleared ad hoc.ApplyLayout currently serves all three, and D13 came directly from that conflation.TriangleJump selecting on layout state rather than on focus state throughout (D10 fixed one branch of this; the rest still reads focus).Not proposing a rewrite — the current code is correct as far as eight rounds of review can establish, and it ships. But the next feature landing in this seam should probably pay for the separation first, and #392 (song grid rebuild architecture) is adjacent enough that they might want designing together.
Filed unclaimed, for triage. Raised on PR #453 as well so it is visible to whoever reviews it.
None.
No comments.