# Campaign OP slice OP5 (the Chat tab) — combined dual-lens review

**Commit under review:** `e71e5a96` "feat(ui): Campaign OP slice OP5 — the Chat tab"
**Reviewer:** combined mechanism + blast-radius lens, 2026-08-11
**Mode:** read-only (no build, no test run, no launch)
**Verdict: APPROVE-WITH-FIXES** — 1 MUST-FIX, 3 SHOULD-FIX, 15 NOTES.

The retail port itself is correct. Every byte-level claim the slice makes was
re-derived independently from the pseudo-C and the committed fixtures and every
one held, including the research-ambiguity call the implementer settled. The
MUST-FIX is a binding-pattern slip (one of the three apply closures does not push
its own widget), not a fidelity error.

---

## MUST-FIX

### M1 — the two opacity rows' `apply` closures never refresh their OWN slider; Reset leaves the thumb desynced from the value

`src/AcDream.App/UI/Layout/ChatOptionsPageController.cs:328-348`

```csharp
defaultRow = new FloatOptionRow(
    initialDefault, bindings.DefaultOpacityDatDefault,
    apply: value =>
    {
        bindings.SetDefaultOpacity(value);
        activeRow!.RefreshFromLink(bindings.CurrentActiveOpacity());   // pushes the OTHER slider
    },                                                                  // never slider1
    read: bindings.CurrentDefaultOpacity,
    refresh: value => slider1.SetScalarPosition(value));
```

This breaks the OP4 binding pattern the slice claims to follow. Both other
implementations push their own widget FIRST inside `apply`:

- `src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs:379-385` —
  `apply: value => { checkbox.Selected = value; bindings.SetOption(...); }`
- this same file, `ChatOptionsPageController.cs:451-456` (the filter block) —
  `apply: value => { block.SetCurrentValue(low, high); bindings.SetFilter(...); }`

`FloatOptionRow.RestoreSavedValue`/`RestoreDefaultValue`
(`OptionPageModel.cs:~236-247`) only call `_apply`, never `_refresh` — exactly
like `BoolOptionRow`, which is safe *because* its `apply` does the widget push.

**Reachable failure.** Drag the Default slider DOWN only. The link
(`ChatOpacityLink.SetDefault`) leaves Active untouched, so `activeRow.Changed`
is false and `defaultRow.Changed` is true. Click **Reset** →
`OptionPage.Reset()` (`OptionPageModel.cs:~465`) restores only `defaultRow` →
`_apply(saved)` → the live opacity reverts (windows visibly change) and
`slider2` gets refreshed — but `slider1`'s thumb stays where the user dragged
it. Symmetric case: drag Active UP only, then Reset. The same desync occurs on
the tab-switch-away path (`OnHidden() => Reset()`), and is only healed later by
`OnShown() => Apply() → SaveCurrentValue() → _refresh`.

`Defaults()` happens to self-heal because it restores *every* row in sequence and
each row's apply refreshes the other one — pure luck, not design.

**Not covered by anything.** No test asserts slider thumb position after
Reset/Defaults (`ChatOptionsPageControllerTests.cs:428-447` checks
`row.Current` and the bindings, not the widget), and the connected gate script
step 11 (`docs/research/2026-08-11-campaign-op-test-script.md`) drags *both*
sliders, which is precisely the case that self-heals.

**Fix shape:** push the row's own slider inside each apply closure (mirroring
OP4), ideally from the post-link truth (`bindings.CurrentDefaultOpacity()`)
rather than the raw `value`, so a link-adjusted value can never leave the thumb
lying. Add one test: single-slider drag → `Reset()` → assert
`slider.ScalarPosition`.

---

## SHOULD-FIX

### S1 — full `settings.json` read+rewrite on every slider drag tick

`src/AcDream.App/UI/RetailUiRuntime.cs:2112-2121` wires both opacity setters to
`SaveChatOpacity()` (`RetailUiRuntime.cs:696-707`), which runs
`store.LoadChat()` (open + `JsonDocument.Parse` of the whole file) followed by
`store.SaveChat(...)` → `SaveSection` → `File.WriteAllText`
(`SettingsStore.cs:538`).

That closure runs once per **mouse-move event** while a thumb is held:
`UiScrollbar.cs:507-511` (`MouseMove when _draggingThumb`) →
`ChangeScalarPosition` → `ScalarChanged` → `FloatOptionRow.SetCurrentValue` →
`apply`. A drag across the track is therefore dozens-to-hundreds of synchronous
whole-file JSON round trips on the UI thread.

Retail does not do this: the structure research doc §3.5 documents the
character-options blob behind a **dirty timer**, i.e. retail batches. The
retail-faithful shape here is mark-dirty + flush on drag end / Apply / page
hide / panel close. (The filter checkboxes are discrete clicks and are fine as
written.)

### S2 — 240–260 px filter blocks inside a viewport that culls whole rows

`src/AcDream.App/UI/UiScrollablePanel.cs:69`

```csharp
child.Visible = top >= -0.5f && top + child.Height <= Height + 0.5f;
```

with the class's own doc (`UiScrollablePanel.cs:8-12`): *"clips whole rows
because the UI renderer does not have a scissor stack yet."*

Until OP5 every row in this viewport was 8–36 px, so the all-or-nothing cull
read as ordinary row-granular scrolling. OP5 inserts five self-sized blocks of
12×20 = 240 px and 13×20 = 260 px into a 560 px viewport whose total content is
roughly 1.5 k px (6 headers @22 + 2 sliders @20/36 + 6 separators @8 + five
blocks). At most scroll offsets a whole block straddles the viewport edge and
therefore **vanishes entirely** rather than clipping — a 260 px pop per
scroll-step, on a page that must be scrolled to reach section 6.

The connected gate script asks the user to verify exactly this
("each block is exactly tall enough to show all of its own rows with **no
clipping** and no dead space", and reports "any block whose height looks
clipped"), so this is very likely to come back as a gate failure attributed to
the self-sizing work rather than to the viewport.

There is no register row for the whole-row cull today (searched
`retail-divergence-register.md` for scissor/whole-row — AD-17/AP-117 are the 3-D
clip rows, unrelated). Either clip this viewport properly or file the row +
issue before the gate.

### S3 — the new persistence key has no round-trip test, and neither does the new seed

`SettingsStore.cs:200` (load) / `:587` (save) add `chatWindowMainFilter`, and
`RetailUiRuntime.cs:926-928` adds the `MountChat` seed for window 0. Neither is
pinned:
`tests/AcDream.UI.Abstractions.Tests/Panels/Settings/SettingsStoreTests.cs:311+`
covers `ChatWindow1Filter`..`ChatWindow4Filter` only, and
`SaveChat_then_LoadChat_round_trips_all_fields` (`:279`) predates the field.
Two-line addition; the floaty test is the template.

---

## NOTES (verified claims + forward hazards)

**N1 — mechanism item 2 (ONE 64-bit mask, not a 128-bit high:low pair):
CONFIRMED. The implementer's reading is right.**
Declaration: `UIOption_CheckboxBitfield64::AddChild(class UIOption_CheckboxBitfield64* this, uint64_t arg2, uint32_t arg3, uint32_t arg4)`
@ `0x004862A0` (`acclient_2013_pseudo_c.txt:146466`); its body stores
`var_128 = arg2` (low) and `var_124 = *(uint32_t*)((char*)arg2)[4]` (high) into
one `ChildInfo` — a single 64-bit stack argument split by BN's usual
push-pair artifact. `SetDefaultValue` is likewise `uint64_t` @ `0x00485780`.
Independent confirmation of the split *direction*: the Society row is emitted as
`var_20_11 = 1; AddChild(..., 0, 0, 0)` @ `0x0049FEFB`/`0x0049FF01` — high dword
1, low dword 0 → `0x0000000100000000`, byte-identical to the committed literal
(`ChatOptionsPageController.cs:120`). Every other row is preceded by
`var_20_N = 0`. `SplitMask`/`CombineMask`
(`ChatOptionsPageController.cs:472-477`) are the correct inverse pair. **A wrong
reading here would have mis-masked every row; it did not.**

**N2 — the five blocks vs §5.2's byte-decoded inventory: exact, two blocks
spot-checked in full.**
Main window inline block `0x0049FDD1`–`0x0049FF23`: `SetDefaultValue(0xfbffffff, 0)`,
`SetUserData(8)`, then **12** `AddChild` calls — `0x600040, 0x20080, 0x1004,
0x18, 0x40c00, 0x80000, 0x8000000, 0x10000000, 0x20000000, 0x40000000,
[hi=1/lo=0], 0x4000000` — no Gameplay row. Floaty-4 inline block
`0x004A0000`–`0x004A0176`: `SetUserData(5)`, default `(0x78000000, 0)`, **13**
calls, the same 12 preceded by `0x83912021`. Helper
`AddCheckboxBitfield64Option @0x0049EDA0`: case 2 → `0x101c`, case 3 →
`0x40c00`, case 4 → `0x80000`, case 5 → `0x78000000`, all falling through
`label_49EE4F` into the Gameplay `AddChild`; case 8 sets `0xfbffffff` and breaks
past it. All of that matches `FilterRows`/`FilterBlocks`
(`ChatOptionsPageController.cs:107-151`) and `ChatWindowState`'s new named
constants (`ChatWindowState.cs:82-86`) exactly. Template indices also confirmed
against the committed fixture: `options_panel_2100006E_1000018D.json` ListBox
`0x1000050D` templates 0..8 = `0x10000216 / 0x10000217 / 0x10000218 /
0x1000021A / 0x10000222 / 0x10000220 / 0x1000021D / 0x10000221 / 0x10000520`, so
header 0, separator 1, unlabelled slider 3, labelled slider 6, bitfield 8 are all
right (research §1.5's table).

**N3 — `GetDIDFromEnumStatic(0x16, 2)` argument order: CONFIRMED.**
`GetDIDFromEnumStatic(__return, arg2, arg3)` @ `0x00413910` forwards to
`GetDIDFromEnum(this, __return, arg3 := arg2, arg4 := arg3)`; the body
(`0x00413940`) runs `EnumIDMap::EnumToDID(masterMap, arg4)` **first**
(`0x004139A1`) and `EnumToDID(subMap, arg3)` second (`0x004139E6`). With the call
being `(0x16, 2)`: master[2] → submap, submap[0x16] → DID. That is exactly
`RetailDataIdResolver.Resolve(dats, enumValue: 0x16, enumCategory: 2)`
(`src/AcDream.Content/RetailDataIdResolver.cs:12-27`) as
`ChatOptionsDatDefaults.cs:40-43,73` calls it. Defaults 0.5/1.0 are pinned
against the live installed DAT (`ChatOptionsDatDefaultsTests.cs:32-34`, skips
gracefully without DATs) and degrade to the documented `ChatInterface`
constructor fallback, never to an invented value.

**N4 — AP-195's LED swap: semantics AND target element are right.**
`Refresh @0x004859C0` computes any-set (`ebx == 0`) and all-set (`var_d_1`),
writes attribute `0x0E` = any-set on checkbox `0x10000219`, and **only while
any-set** calls `SetMediaImageForState(GetFirstChildElement(checkbox),
0x10000082-if-all-else-0x10000083, 1, 6)`. `SetMediaImageForState @0x00463BE0`
writes exactly **one** state — id `arg4` = 6 — not a 1..6 range. The committed
fixture shows why that is equivalent to our port: LED child `0x10000328` state 6
is `Highlight` and its authored image already IS `0x06004D17`
(= property `0x10000082`), while `Ghosted` is `0x06004D19` (= `0x10000083`); a
checked button resolves to `Highlight` via
`UiButton.UpdateVisualState`/`UiButtonStateMachine`. Our `FaceFileOverride` is
set on the checkbox `UiButton`, and `DatWidgetFactory.BuildButton:718-724,765-771`
makes that button's *face* be the 13×13 LED child (checkbox `StateMedia` is
empty, exactly one stateful face child), with `FaceLeft/Top/Width/Height` =
`0/1/13/13`, and `_faceSegments` empty so `UiButton.cs:347`'s override branch is
the one taken. All-set / partial / unset render identically to retail.

**N5 — the self-sizing half is real.** `CreateChildren @0x00485DF0` ends
`ResizeTo(GetWidth(), CalculatePaperSize(&vtable, 0, 0xffffffff))` then
tail-jumps `Refresh`. `UiCheckboxBitfield64.cs:237` grows `Height` only and
leaves `Width` at the authored 272 — the right shape. Minor inexactness:
`CalculatePaperSize` is retail's own paper metric (list insets included);
we use the exact sum of row heights. Non-blocking, worth a line in the class doc.

**N6 — retirement note truthful, AP-187 broadening accurate.** The AP-195 row is
deleted in this same commit, the §3 header count goes 138 → 137, and AP-187 is
rewritten (not duplicated) to cover the main window's filter plus the new live
write path, citing `2026-08-09-chat-retail-window-shell.md` §4.1/§4.4/§6.1 and
the CH6f plan row rather than restating them — exactly the "cite, don't
duplicate" instruction in the plan's OP5 section. Register bookkeeping for this
slice is clean.

**N7 — fixture regeneration is strictly additive (the OP2 lesson holds).**
`git diff --numstat` over `tests/AcDream.App.Tests/UI/Layout/fixtures/`: 33 files,
**0 deleted lines**. Every added line is one of exactly two shapes
(`"LedCheckedSprite": N,` / `"LedUncheckedSprite": N,`), and only ONE element
across all 33 files carries non-zero values — `100683031`/`100683033`
(`0x06004D17`/`0x06004D19`) on `0x10000520` in `options_2100002B.json`, precisely
as AP-195 documented. No pre-existing value changed anywhere, so no conformance
suite can shift meaning. (Cosmetic: the commit message says "all 19 committed
layout fixtures"; it is 33.)

**N8 — `UiButton.FaceFileOverride` blast radius: none.** Only two writers, both
in `UiCheckboxBitfield64.ApplyRowVisualsForMask` (`UiCheckboxBitfield64.cs:324,328`);
default `null` everywhere else; consulted at exactly one draw site
(`UiButton.cs:347`) in the single-face branch. Every other `UiButton` instance in
the codebase is bit-identical to before. One silent-failure caveat: if a future
bitfield row template ever authors **more than one** stateful face child,
`_faceSegments` becomes non-empty and the override is ignored with no log
(`UiButton.cs:340-346`). Not reachable with the shipped `0x10000521`.

**N9 — `ElementInfo` threading is safe.** The two reads sit in the same
"recompute from the current effective state every call" block as
`TabTable`/`TemplateList`/`ScrollbarElementId` (`ElementReader.cs:509-531`), so
base+derived merge is picked up automatically; `ReadReferencedElementId`
(`ElementReader.cs:602-612`) accepts `Enum`/`DataId`/`Integer`, which covers the
Kind-2 DataId properties. Only `UiCheckboxBitfield64` consumes the fields
(`DatWidgetFactory.cs:165-166`), so an unrelated element that happened to author
`0x10000082` would populate a field nobody reads.

**N10 — `AddPrebuiltRow` changes nothing for existing template lists.** It is a
literal extraction of `AddItemFromTemplateList`'s tail (`UiTemplateListBox.cs:179-217`);
the caller now delegates. Only new caller is `ChatOptionsPageController.cs:442`.
Other `AddItemFromTemplateList` consumers are the Character tab
(`CharacterOptionsPageController.cs:285,308,322`) and OP5's own header/separator/
slider rows; the component/effect/examine row factories use their own helpers.
Two new tests pin both the stacking and the "a row added AFTER a prebuilt block
stacks below the block's FINAL height" property.

**N11 — the shared scrollbar id: OP5 is right, and OP6 MUST repeat it (with a
sharper test).** `0x10000201` is authored in **two** page slots of the host tree:
under the Config slot `0x10000213` (ListBox `0x10000200`, 8 templates) and under
the Chat slot `0x1000050C` (ListBox `0x1000050D`, 9 templates). `ImportedLayout`
is a flat `Dictionary<uint, UiElement>` written as `byId[info.Id] = w` during a
depth-first build (`LayoutImporter.cs:97,120`), i.e. **last-write-wins**, and the
Chat slot is the LAST top-level child (index 9) while Config is index 7 — so a
flat `FindElement(0x10000201)` returns the **Chat** instance today. OP5's scoped
lookup from `PageSlotElementId` (`ChatOptionsPageController.cs:224-227`) is
correct and future-proof, but currently returns the same object the flat lookup
would. Consequences:
1. **OP6 must scope from `0x10000213`.** A plain `layout.FindElement(0x10000201)`
   in the Config controller will bind Config's scroll model onto **Chat's**
   scrollbar, and nothing will fail loudly.
2. `ScrollbarLinkage_ModelPointsAtTheChatListBoxScroll`
   (`ChatOptionsPageControllerTests.cs:566-588`) asserts through the FLAT lookup,
   so it passes for the wrong reason and would not catch a de-scoping
   regression. Assert through `UiElement.FindDescendant(chatPageSlot, ...)`.
The same shared-id shape applies to `0x100001FC/FD/FE` (Apply/Reset/Defaults),
present in all three page slots and already scoped per page by
`OptionsPanelController.cs:214-222` — including `ChatPageId`, so OP5 inherits
correct per-page ghosting for free.

**N12 — plan cross-reference is stale.** The plan's OP5 section
(`docs/plans/2026-08-10-options-panel-campaign.md:287`) cites "lane A §8's
byte-decoded masks", but §8 of `2026-08-10-options-panel-structure.md` is
*Configure Keyboard*; the masks are §5/§5.2. The code cites §5.2 correctly
(`ChatOptionsPageController.cs:13,104,423`). Fix the plan pointer.

**N13 — naming reads backwards.** `UiCheckboxBitfield64.UncheckedLedSprite` /
`ElementInfo.LedUncheckedSprite` mean "checked but only PARTIALLY set"; a genuinely
unchecked row gets **no** override at all. The doc comments say so, but the names
mislead at the call site. `PartialLedSprite` would match retail's own semantics.

**N14 — no invented user-visible text.** Every string is resolved by hashing
retail's own symbol name through `DatStringResolver.ComputeHash`, and all of them
exist in the pseudo-C globals: the six `ID_ChatOption_*_Section` headers, the 13
`ID_ChatOption_TextFilter_*` labels (including retail's own "Allegience"
misspelling, correctly used as the hash key with the correctly-spelled table
value), their `_Desc` tooltip counterparts, and
`ID_UI_Value_Transparent`/`ID_UI_Value_Opaque`. Every resolution failure logs and
renders empty rather than falling back to English
(`ChatOptionsPageController.cs:262-268,364-368,429-433`).

**N15 — OP4-pattern compliance otherwise holds.** `OnShown() → Apply() →
SaveCurrentValue()` re-reads the live source and pushes it to the widget for both
new row types (`OptionPageModel.cs` `FloatOptionRow.SaveCurrentValue`,
`BitfieldOptionRow.SaveCurrentValue`), with tests
(`ChatOptionsPageControllerTests.cs:407-425,505-521`). Apply/Reset ghosting is
wired per page including `ChatPageId` (`OptionsPanelController.cs:214-235`).
`UiCheckboxBitfield64.SetCurrentValue` deliberately does not fire `ValueChanged`,
so a re-seed cannot loop back as a user edit (pinned at
`OptionsPanelLayoutConformanceTests.cs:~468-479`). Retail's slider labelling is
matched too: `InitOptions` calls `SetSliderLabel` for the **labelled** slider only
(`0x0049FD37`), and the port sets range captions on row 2 only
(`ChatOptionsPageController.cs:311-315`) and no row caption on either — retail
sets none either. End-to-end live routing verified: checkbox click →
`ToggleRow` → `ValueChanged` → `BitfieldOptionRow.SetCurrentValue` → apply →
`ChatWindowState.SetFilter` (a real write for window 0 since CH6a/b,
`ChatWindowState.cs:158-167`, bumping `Revision`) + `SaveChatWindowFilters` →
`ChatWindowController.GetTranscriptLines` re-lays out because its cache keys on
the filter value itself (`ChatWindowController.cs:669-676`). Mount ordering is
safe: `MountChat` (`:366`) → `MountFloatingChatWindows` (`:367`) →
`MountOptionsPanel` (`:374`), so `SaveChatWindowFilters`' all-five write can
never persist default floaty filters over stored ones. `ChatSettings`' new
parameter is inserted mid-list in a positional record, but both construction
sites use named arguments (`SettingsStore.cs:186`, `SettingsStoreTests.cs:282`),
so no silent argument shift.

---

## Summary table

| ID | Class | One-line |
|---|---|---|
| M1 | MUST-FIX | Opacity `apply` closures don't push their own slider; single-row Reset desyncs the thumb from the value |
| S1 | SHOULD-FIX | Whole `settings.json` load+rewrite per slider drag tick; retail batches behind a dirty timer |
| S2 | SHOULD-FIX | 240–260 px blocks in a viewport that culls whole rows — blocks blink out mid-scroll; no register row |
| S3 | SHOULD-FIX | No round-trip test for `chatWindowMainFilter`, none for the new `MountChat` seed |
| N1–N15 | NOTE | See above — masks/defaults/argument-order/AP-195/fixtures all verified; OP6 scrollbar-scoping hazard in N11 |