acdream/docs/research/2026-08-11-op2-review-blast.md

# OP2 review — regression / blast-radius lens

**Commit under review:** `df9c7a35` "feat(ui): Campaign OP slice OP2 — tab control,
template ListBox, UIOption widget mappings"
**Reviewer lens:** what OP2 breaks OUTSIDE its own scope (Options panel).
**Date:** 2026-08-11
**Verdict: REJECT** — two independent MUST-FIX regressions on shipped panels, and a
test harness structurally incapable of observing either.

---

## 0. Executive summary

OP2's own acceptance criterion is written into the code it added:

> `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:103` —
> "*(OP2 acceptance: no behavior change to any existing widget)*"

That criterion is **falsified on two independent axes**, and the justification for one
of them is a factual claim about the codebase that is demonstrably wrong:

> `DatWidgetFactory.cs:99-101` — "*an ORDINARY Type-5 ListBox that authors no template
> array (**none currently reach this factory** …) keeps falling to the generic
> UiDatElement fallback unchanged*"

**Ten** Type-5 elements reach that factory today, in five shipped panels, and **every
one of them authors a non-empty template array**. Separately, **five** Type-8 elements
in four shipped panels change widget class.

Total blast radius: **15 elements across 7 shipped, user-gated panels** — character
sheet, combat, spellbook/component book, vendor, examination, effects (positive +
negative), mini-game — silently change widget class in production.

The 12,770-test green suite is a **structural false negative**: the fixture path never
calls the code that populates the new fields, so every fixture-driven test takes the
pre-OP2 branch while production takes the new one.

This is the failure mode `docs/research/2026-08-05-c4-closeout-handoff.md` names as
binding: *a contract asserting a mechanism that does not exist*, plus *inferring a fact
you can observe*.

The new Options-panel work itself (fixtures, tab table read, template list read, the
four `UIOption_*` mappings, the 25 tests) is sound. The problem is entirely in the
un-gated widening of two `switch` arms onto pre-existing authored data.

---

## 1. Fixture-wide enumeration (the requested sweep)

Method: parsed all **32** committed fixtures under
`tests/AcDream.App.Tests/UI/Layout/fixtures/` (27 pre-existing + 5 new), walked every
`ElementInfo` node, and recorded (a) the node `Type`, (b) whether any state authors dat
property `0x2E` / `0x64` / `0x72`, and (c) whether the JSON carries the new serialized
fields.

### 1.1 Type-8 (`UIElement_TabControl`) — 6 occurrences, 5 of them pre-existing

| Fixture | Element | Children | Authors `0x2E`? | Tab entries | Pre-OP2 widget | Post-OP2 widget |
|---|---|---|---|---|---|---|
| `character_2100002E.json` | `0x10000227` (**panel ROOT**) | 7 | yes | **3** | `UiDatElement` | `UiTabControl` |
| `combat_21000073.json` | `0x100000A2` | 16 | yes | **8** | `UiDatElement` | `UiTabControl` |
| `spellbook_21000034.json` | `0x100002A8` (**panel ROOT**) | 5 | yes | **2** | `UiDatElement` | `UiTabControl` |
| `vendor_21000012_100000B7.json` | `0x100000B8` | 6 | yes | **3** | `UiDatElement` | `UiTabControl` |
| `vendor_21000012_100000B7.json` | `0x1000008D` | 0 | **no** | 0 | `UiDatElement` | `UiTabControl` |
| `options_2100002B.json` | `0x10000208` | 10 | yes | 4 | *(new)* | `UiTabControl` ✅ |

Authored tab tables actually present in the pre-existing panels (verbatim from the
fixtures, `{0x30 button, 0x31 page, 0x32 isDefault}`):

- character `0x10000227`: `(0x10000228→0x1000022B, default)`, `(0x10000229→0x1000022C)`,
  `(0x100003F8→0x100003F9)`
- combat `0x100000A2`: 8 rows, `(0x10000063→0x1000006A, default)` … `(0x10000382→0x10000383)`
- spellbook `0x100002A8`: `(0x100002A9→0x100002AC, default)`, `(0x100002AA→0x100002AD)`
- vendor `0x100000B8`: `(0x100000B9→0x1000007C, default)`, `(0x100000BA→0x10000084)`,
  `(0x100000BB→0x1000008D)`

### 1.2 Type-5 (`UIElement_ListBox`) — 16 occurrences, 10 of them pre-existing

**Every pre-existing Type-5 element authors a NON-EMPTY property `0x64`.**

| Fixture | Element | `0x64` rows | `0x72` | Controller constant | Post-OP2 (production) |
|---|---|---|---|---|---|
| `character_2100002E.json` | `0x1000023D` ×2 | **5** | `0x1000023E` | `CharacterStatController.ListBoxId` | `UiTemplateListBox` |
| `character_2100002E.json` | `0x10000532` | **1** | `0x10000533` | — | `UiTemplateListBox` |
| `effects_positive_2100001B.json` | `0x10000123` | **1** | `0x10000124` | `EffectsUiController.ListId` | `UiTemplateListBox` |
| `effects_negative_2100001B.json` | `0x10000123` | **1** | `0x10000124` | `EffectsUiController.ListId` | `UiTemplateListBox` |
| `examine_2100006B_100005F2.json` | `0x10000149` | **1** | — | `AppraisalUiController.CreatureStatsListId` | `UiTemplateListBox` |
| `examine_2100006B_100005F2.json` | `0x1000032D` | **1** | — | `AppraisalUiController.SpellFormulaListId` | `UiTemplateListBox` |
| `examine_2100006B_100005F2.json` | `0x10000335` | **1** | — | `AppraisalUiController.CreatureExtraListId` | `UiTemplateListBox` |
| `mini_game_2100001E.json` | `0x10000174` | **1** | — | — | `UiTemplateListBox` |
| `spellbook_21000034.json` | `0x10000464` | **2** | `0x10000465` | `SpellbookWindowController.ComponentListId` | `UiTemplateListBox` |
| `options_*` (3 new) | `0x100001FA` / `0x10000200` / `0x1000050D` | 3 / 8 / 9 | yes | *(new)* | `UiTemplateListBox` ✅ |

### 1.3 The four `0x100000xx` UIOption class ids — **zero** pre-existing occurrences

| Class id | Occurrences | Files |
|---|---|---|
| `0x10000036` CheckboxSlider | 2 | `options_2100002B.json` only |
| `0x10000037` Slider | 4 | `options_2100002B.json` only |
| `0x10000038` Menu | 1 | `options_2100002B.json` only |
| `0x10000044` CheckboxBitfield64 | 1 | `options_2100002B.json` only |

**Clean.** These four arms are the only genuinely additive part of the switch change.
No pre-existing layout carries any of them.

### 1.4 New-field presence per fixture (fixture-provenance check)

`System.Text.Json` with `IncludeFields = true` serializes *every* public field, so a
fixture generated by the current generator necessarily contains `Outline`,
`OutlineColor`, `TabTable`, `TemplateList`, and `ScrollbarElementId` on every node.

| Group | Files | `Outline`/`OutlineColor` | `TabTable`/`TemplateList`/`ScrollbarElementId` |
|---|---|---|---|
| The 5 new Options fixtures | `options_2100002B`, `options_gameplay_2100002A`, `options_character_21000028`, `options_chat_2100005C`, `options_config_21000029` | **present** | **present** |
| `chat_2100006f.json` | regenerated at CH6a | present | absent |
| The other 26 pre-existing | all remaining | **absent** | **absent** |

---

## 2. MUST-FIX findings

### MUST-FIX 1 — Type-8 arm silently re-classes 5 elements in 4 shipped panels

**Site:** `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:110`
```csharp
8    => new UiTabControl(info.TabTable),
```
Unlike the Type-5 arm, this one carries **no guard at all** — every Type-8 element in
every layout now becomes a `UiTabControl`.

`UiTabControl` (`src/AcDream.App/UI/UiTabControl.cs:42`) derives from bare `UiElement`.
`UiDatElement` (`src/AcDream.App/UI/Layout/UiDatElement.cs:31`) is
`UiElement, IUiDatStateful` and its constructor sets `ClickThrough = true`
(`UiDatElement.cs:111`). Four independent consequences:

**(a) Lost background art — a straight rendering regression.**
Vendor `0x1000008D` is 800×20 and authors a DirectState sprite:
`StateMedia[""] = (100687632, DrawMode 1)`. `RetailUiRuntime.cs:2287` documents it by
name: *"backdrop element `0x1000008D` provides its own fill"*. It has **zero children**
and **no `0x2E` property** — it is pure chrome that happens to carry retail Type 8.
`UiTabControl` has no media plumbing whatsoever, so **the vendor panel's backdrop fill
stops drawing.** This is the clearest single defect in the commit.

**(b) `ClickThrough` inverts `true → false` on all five.**
`UiElement.ClickThrough` is an auto-property defaulting to `false`
(`UiElement.cs:159`); `UiElement.HitTest` gates on it at `UiElement.cs:582-583`
(`if (ClickThrough) return null; return OnHitTest(...) ? this : null;`), and
`OnHitTest` defaults to a plain bounds test (`UiElement.cs:421-422`). Two of the five
are **panel roots** (character `0x10000227`, spellbook `0x100002A8`), so the entire
character sheet and the entire spellbook now claim every pointer hit inside their
bounds instead of passing unclaimed clicks through to the window shell behind them.

**(c) Lost `IUiDatStateful` state propagation.**
`LayoutImporter.cs:168-169` runs `stateful.TrySetRetailState(...)` only for
`IUiDatStateful` widgets; `UiDatElement.TrySetRetailState` recurses into children
(`UiDatElement.cs:98-99`). `UiTabControl` does not implement the interface, so the
authored default-state propagation into the 7 / 16 / 5 / 6 children of these four tab
hosts silently stops. This is the exact mechanism the comment at
`LayoutImporter.cs:164-167` says the hook exists to preserve.

**(d) Import-time visibility + click rebinding on controller-owned tabs.**
`UiTabControl.OnChildrenAttached` (`UiTabControl.cs:65-81`) runs *during*
`LayoutImporter.Build`, before any controller wiring, and calls
`RetailTabBinding.SetClick` (which also sets `ClickThrough = false` on each tab button,
`RetailTabBinding.cs:16`) plus `SwitchTo(default)` → `page.Visible = active` and
`RetailTabBinding.SetOpen`. All four panels already own these tabs
(`CharacterStatController.cs:428-432`, `SpellbookWindowController.cs:138-139`,
`VendorUiController.cs:587-589`).

Mitigating detail, and it is *luck*, not design: on 3 of the 4 panels the tab table's
ids do **not** match the resolved child ids, so `FindDirectChild` misses and `SwitchTo`
is inert — vendor's table names pages `0x1000007C/0x10000084/0x1000008D` while the
controller and the actual children use `0x100000BC/0x100000C4/0x100000CD`; combat's
table names `0x1000006A..0x10000383` while the children are `0x100000AA..0x100005C3`.
Only spellbook and character resolve, and both controllers happen to re-assert
visibility afterwards. **Nothing in the commit establishes or tests this ordering.**

Worth flagging on its own: `CharacterStatController.cs:439-449` carries an explicit
comment — *"WHY this cannot be done in the importer … The controller is the correct and
only place for initial tab-page selection"* — that OP2 now contradicts for four panels
without amending it.

### MUST-FIX 2 — Type-5 guard rests on a false premise; 10 elements flip in production

**Site:** `src/AcDream.App/UI/Layout/DatWidgetFactory.cs:104-105`
```csharp
5 when info.TemplateList.Count > 0 => new UiTemplateListBox(
    info.TemplateList, info.ScrollbarElementId),
```
The guard is correct *in the fixture path* and useless *in production*, because
`info.TemplateList` is populated by different code on the two paths:

- **Fixture path:** `FixtureLoader.LoadInfos` (`FixtureLoader.cs:253-265`) deserializes
  straight into `ElementInfo` and hands it to `LayoutImporter.Build`.
  `ApplyCanonicalLegacyProjection` is **never called**. Stale fixtures carry no
  `TemplateList` key → the field initializer's empty list survives → guard is `false`
  → old `UiDatElement` behavior. Tests are green.
- **Production path:** `LayoutImporter.ToInfo` calls
  `ElementReader.ApplyCanonicalLegacyProjection(info)` (`LayoutImporter.cs:465`), which
  runs `info.TemplateList = ReadTemplateList(info)` from property `0x64`. Guard is
  `true` for all ten → `UiTemplateListBox`.

Section 1.2 shows all ten pre-existing Type-5 elements author 1–5 valid
`{0x63 layoutDid, 0x62 elementId}` rows. Six of them are named controller constants
(`AppraisalUiController.CreatureStatsListId` / `CreatureExtraListId` /
`SpellFormulaListId`, `CharacterStatController.ListBoxId`, `EffectsUiController.ListId`,
`SpellbookWindowController.ComponentListId`).

Consequences: same `IUiDatStateful` loss and same media loss as MUST-FIX 1, plus a
**spurious child injection** — `UiTemplateListBox`'s constructor unconditionally does
`base.AddChild(_viewport)` (`UiTemplateListBox.cs:94`), so every one of these ten list
boxes gains a full-size anchored `UiScrollablePanel` as its first child. Since
`UiPanel` defaults `ClickThrough = false` (confirmed by the comment at
`UiPanel.cs:157-159`), that transparent viewport participates in hit-testing across the
whole list-box rect, competing with the `UiItemList` each controller adds afterwards on
an untested Z-order tie-break.

Mitigating: the consumers all use `FindElement(...)` returning `UiElement?` with an
`is UiItemList` test plus an else-branch (`EffectsUiController.cs:90-94`,
`SpellbookWindowController.cs:196-199`, `AppraisalUiController.cs:169`,
`:241-243`, `:313-315`), so no controller *fails to mount*. The damage is confined to
rendering, hit-testing, and state propagation — but it is unreviewed and unmeasured.

### MUST-FIX 3 — the harness cannot observe either change (the root enabler)

`ApplyCanonicalLegacyProjection` has exactly two call sites, both DAT-only:
`LayoutImporter.cs:465` (`ToInfo`) and `ElementReader.cs:410` (`Merge`). The fixture
path reaches neither. Therefore:

- Every fixture-driven test exercises the **pre-OP2** branch for all 15 affected
  elements while production exercises the **post-OP2** branch. The "12,770 passed /
  0 failed, zero regressions" claim in the commit message is true and meaningless for
  this change.
- The one existing test that touches an affected element,
  `RetailFixtureConformanceTests.cs:100`
  (`{ FixtureLoader.LoadCharacterInfos, 0x10000227u, 8u, 300f, 600f, 60 }`), asserts the
  **`ElementInfo.Type`**, not the built widget class — it already knew `0x10000227` is
  Type 8 and still could not catch this.
- `RetailLayoutFixtureGenerator.cs` now regenerates **all 32** layouts. The moment the
  coordinator runs `ACDREAM_REGENERATE_UI_FIXTURES=1`, the 27 stale fixtures gain
  populated `TabTable`/`TemplateList`, and the suite flips to production shapes for all
  15 elements in one commit, with no test written to explain the diff.

Answering the task's forward-looking question directly: **no committed test depends on
fields present only in new-shape fixtures.** The exposure is the inverse and worse —
tests currently depend on those fields being *absent*.

**Minimum fix shape (recommendation, not a mandate):** narrow both arms so they cannot
reach a pre-existing element — e.g. gate on the Options LayoutDesc ids, or on the
authored-tab-table ids actually resolving to direct children, or (cleanest) require
`info.TabTable.Count > 0 && every ButtonElementId/PageElementId resolves` before
choosing `UiTabControl`, and make `UiTabControl`/`UiTemplateListBox` carry the same
media + `IUiDatStateful` behavior `UiDatElement` provides. Whatever the shape, land a
test that builds the four affected pre-existing layouts **through the DAT-path
projection** (call `ApplyCanonicalLegacyProjection` over the fixture tree before
`Build`) and asserts the resolved widget class for all 15 ids.

---

## 3. SHOULD-FIX

### SHOULD-FIX 1 — `UiTemplateListBox` injects a hit-testable viewport unconditionally

`UiTemplateListBox.cs:94` (`base.AddChild(_viewport)`) runs in the constructor, before
`LayoutImporter.BuildWidget` appends the element's own dat children. The widget does not
set `ConsumesDatChildren`, so authored children become **siblings of the viewport rather
than rows inside it** — silently correct today only because all three Options ListBoxes
author zero children, and a latent trap for OP3+ the moment one doesn't. Combined with
`UiPanel`'s `ClickThrough = false` default this is the mechanism behind MUST-FIX 2's
hit-testing exposure.

### SHOULD-FIX 2 — the tab table's ids are base-layout ids on inherited layouts

`UiTabControl.FindDirectChild` (`UiTabControl.cs:105-111`) matches
`child.DatElementId` against the raw `0x2E` values. On three of the four pre-existing
panels those values do not correspond to any child in the *incorporated* tree (§2,
MUST-FIX 1(d)) — strongly suggesting property `0x2E` carries **base-layout** ids that
`LayoutImporter.IncorporateChildren` remaps. The Options panel happens to author its
tab table with post-incorporation ids, so OP2's own tests pass. This will bite OP3+
directly, because the Options pages mount cross-layout via `BaseLayoutId`/`BaseElement`
— exactly the case where the remap applies. Worth resolving now rather than
rediscovering it as an OP4 mystery.

### SHOULD-FIX 3 — silent no-resolve is indistinguishable from correct behavior

`OnChildrenAttached` and `SwitchTo` both treat a `FindDirectChild` miss as a no-op
(`UiTabControl.cs:70-72`, `:95-99`). A tab control whose entire table fails to resolve
(combat, today) is behaviorally identical to one with no table at all. Given
SHOULD-FIX 2, a diagnostic line on unresolved entries would have surfaced this during
implementation.

---

## 4. NOTEs (verified clean, or informational)

**NOTE 1 — `ElementReader.Merge` handles the three new fields correctly.** Verified.
`Merge` (`ElementReader.cs:344-412`) does *not* add scalar merge rules for `TabTable`,
`TemplateList`, or `ScrollbarElementId`, and does not need to: it merges the **state
property tables** first (`:400-408`, `UiStateInfo.Merge` giving derived-wins per
property) and then calls `ApplyCanonicalLegacyProjection(m)` at `:410`, which recomputes
all three from the merged effective state. Base-only `0x64` survives; derived `0x64`
replaces base wholesale (correct — arrays are whole-value properties, not concatenated);
neither is doubled. **No existing `Merge` behavior changed** — the new field reads are
appended after the existing `Outline`/`OutlineColor` block in
`ApplyCanonicalLegacyProjection` and touch no prior line. This addresses focus point 2
in full: nothing here can drop or double a template list for the OP3+ cross-layout
mounts.

**NOTE 2 — fixture-drift handling is exactly as described.** `git show --stat df9c7a35`
lists 5 fixture files, all `A` (added), zero `M`. The 27 pre-existing fixtures are
byte-untouched by this commit. And per §1.4 the 5 new fixtures **do** carry
`Outline`/`OutlineColor` (plus the three new fields), proving they came from the current
generator rather than being hand-built or stale-sourced. This focus-3 MUST-FIX condition
is **satisfied**.

**NOTE 3 — the 25 new tests are hermetic.**
`OptionsPanelLayoutConformanceTests.cs` contains exactly 25 `[Fact]`s (matching the
commit message) and zero `IDatReaderWriter` / `DatCollection` references; every test
loads a committed fixture through `FixtureLoader`. The generator is env-gated
(`RetailLayoutFixtureGenerator.cs:42-50`, early-returns unless
`ACDREAM_REGENERATE_UI_FIXTURES == "1"`), so it is inert on a machine without DATs. The
csproj copies fixtures by wildcard (`AcDream.App.Tests.csproj:28`,
`UI\Layout\fixtures\*.json`), so the five new files need no registration. **The suite
remains green without the machine DAT.** Focus point 5: clean.

**NOTE 4 — `IUiChildrenAttachedListener` cannot double-fire.** The hook is invoked
exactly once per widget instance, at `LayoutImporter.cs:174-175`, immediately after that
widget's own child loop and before `BuildWidget` returns. Depth-first ordering means a
nested listener fires before its ancestor, which is the correct order for
cross-reference resolution. Nested `LayoutImporter.Build` calls from
`UiTemplateListBox.TemplateResolver` create fresh widget instances, so no re-entry
hazard. Focus point 4's "fire twice / wrong subtree" concern: **not present.**

**NOTE 5 — wheel / drag / clipping forwarding is structurally fine.** Because
`_viewport` is anchored `Left|Top|Right|Bottom` and `UiElement.HitTest` walks children
before gating on the parent (`UiElement.cs:571-583`), pointer and wheel events land on
the `UiScrollablePanel` directly — no forwarding shim is needed, and the scroll model,
per-row clipping, and `LineHeight` quantum are the same `UiScrollablePanel` code the
character sheet and CH6 already exercise. The anchor-capture behavior for a viewport
constructed at 0×0 inside a sized host is *not* covered by any of the 25 tests; low risk,
but unmeasured.

**NOTE 6 — `UiOptionToggleSlider` first-match binding.**
`UiOptionToggleSlider.OnChildrenAttached` (`UiOptionToggleSlider.cs:33-42`) binds the
first `UiButton` and first `UiScrollbar` child. The `else if` is harmless (a child cannot
be both types). Zero pre-existing layouts carry class `0x10000036`, so no blast radius —
noted only because the pattern silently picks the first match rather than the authored
`0x10000219` / `0x1000021C` ids the class doc cites.

**NOTE 7 — per-element allocation in the hot projection path.**
`ApplyCanonicalLegacyProjection` now allocates two fresh `List<>` instances
(`ReadTabTable`, `ReadTemplateList`, `ElementReader.cs:490-497`) for **every element on
every call**, including the ~99% that author neither property, and `Merge` calls it once
per inheritance level. Import is startup-only and not on a frame path, so this is
cosmetic — but an early `TryGetEffectiveProperty` bail before allocating would be free.

**NOTE 8 — TS-72 is correctly filed.** The register hunk
(`docs/architecture/retail-divergence-register.md`) adds TS-72 for
`UiCheckboxBitfield64`'s click-toggle bit math and bumps the count 40 → 41. Correct per
the register rules; the widget is genuinely unreachable today. No comment on the bit
math itself — outside this lens.

---

## 5. Verdict

**REJECT.**

The additive half of OP2 (five fixtures, the three `ElementReader` property readers, the
`Merge` interaction, the `IUiChildrenAttachedListener` hook, the four `UIOption_*` class
ids, the 25 hermetic tests) is well-built and clean under this lens. But two `switch`
arms were widened onto authored data the commit believed was absent, changing the widget
class of 15 elements across 7 shipped, user-gated panels — including a documented
rendering backdrop that now draws nothing — and the harness that reported "zero
regressions" is structurally incapable of seeing any of it. The re-review gate should be
the DAT-path projection test described at the end of §2.