acdream/docs/research/2026-08-11-op5-review.md

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

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

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