acdream/docs/research/2026-08-12-fa3-review-blast.md

Campaign FA slice FA3 — BLAST-RADIUS review

Reviewer lens: the axes the implementer may not have traversed — every consumer of the tables/widgets/seams FA3 touched, not the correctness of the social panel itself (that is the mechanism reviewer's lane).

Target: 0a9ca2f1, 74c3d85d, b6a25110, d7e1cffd on claude/latest-commits-cb0c8f.

Verdict: APPROVE-WITH-FIXES — 1 MUST-FIX, 6 SHOULD-FIX, 1 NIT.

The slice is structurally clean: the catalog/window-name/composition/mount-order seams were all traversed correctly and every table consumer degrades safely. One authored control is dead, and one new per-frame path does live DAT I/O while the panel is closed.

---

MUST-FIX

MF-1 — The Friends and Squelch lists have no scroll driver at all: the authored scrollbars are never wired

SocialFriendsPageController.Bind and SocialSquelchPageController.Bind set listBox.TemplateResolver and stop:

• src/AcDream.App/UI/Layout/SocialFriendsPageController.cs:65
• src/AcDream.App/UI/Layout/SocialSquelchPageController.cs:61

Both ListBoxes author a scrollbar, and FA3's own committed fixture proves it — each scrollbar is a direct sibling of its ListBox under the page root, so a FindDescendant(pageRoot, id) would resolve it trivially:

Page	ListBox	rect	ScrollbarElementId	scrollbar rect
Friends 0x10000513	0x10000517	(8,40) 270x400	0x10000518	(280,40) 16x400, Type 11
Squelch 0x1000054A	0x1000053E	(8,40) 270x430	0x10000543	(280,40) 16x430, Type 11

Every other UiTemplateListBox consumer in the tree wires the model:

• src/AcDream.App/UI/Layout/CharacterOptionsPageController.cs:322
• src/AcDream.App/UI/Layout/ChatOptionsPageController.cs:291
• src/AcDream.App/UI/Layout/ConfigOptionsPageController.cs:378
• src/AcDream.App/UI/Layout/KeyboardConfigController.cs:277

There is no wheel fallback. Wheel scrolling exists only on UiText (src/AcDream.App/UI/UiText.cs:225, :714-718 — WheelScrollEnabled); UiScrollablePanel has no wheel handler. With Model unbound, the viewport's Scroll has no driver whatsoever — the list is not merely awkward to scroll, it is completely unscrollable.

Impact is user-visible at the authored size, not just at large rosters. The panel root 0x1000018F is 300x362. The Friends ListBox starts at y=40 and is 400 tall, so only ~322 px of it is inside the panel to begin with. Every row past ~322 px is off-panel and unreachable. Squelch is worse (430-tall box in the same 362-tall panel).

This is also an undocumented divergence: AD-79 covers the seven inert Friends/Squelch action buttons, not a dead scrollbar. The lists themselves are advertised as live and read-only, and a list you cannot scroll is not fully live.

Fix: two lines per controller, mirroring the four existing sites — resolve the ListBox's own ScrollbarElementId scoped to the page root and assign scrollbar.Model = listBox.Scroll. No register row needed once wired.

---

SHOULD-FIX

SF-2 — The revision-driven rebuild does N live DAT imports under the shared DAT lock, while the panel is closed

SocialPanelController.Tick() is called unconditionally from src/AcDream.App/UI/RetailUiRuntime.cs:559, with no visibility gate. Each AddItemFromTemplateList(0) invokes MountSocialPanel's TemplateResolver (RetailUiRuntime.cs:2626-2642), which takes _bindings.Assets.DatLock and runs a full ImportInfos + Build per row.

FA3 is the first consumer to call that resolver on a repeating schedule, and the first to call it while its panel is hidden. Every pre-existing consumer builds rows exactly once at Bind: KeyboardConfigController builds all six pages once (:255-295), and the three Options page controllers likewise.

So one friend logging in or out (FriendsUpdateType.OnlineStatus → Interlocked.Increment(ref _revision), src/AcDream.Core/Social/FriendsState.cs:61) rebuilds the entire roster: N lock acquisitions and N DAT reads on the UI thread, with the panel closed. The DAT lock is shared with world-mesh streaming.

Suggest gating Tick on window visibility (refresh on OnShown — the IRetainedPanelController hook already exists and is currently unused by this controller) and/or caching the built row template.

SF-3 — Refresh() consumes the revision before building, so one transient resolver miss latches an empty list

SocialFriendsPageController.cs:79-80 and SocialSquelchPageController.cs:75-76 set _lastRevision = <state>.Revision first, then Flush(), then add rows. AddItemFromTemplateList returns null on a resolver miss and both controllers continue/return silently. Because the revision is already consumed and the list has already been flushed, a single failed resolve leaves the roster empty until the next revision bump. The Flush-first ordering makes this strictly worse than the one-shot consumers, which at least fail with their authored content intact. Advance _lastRevision only after a successful rebuild.

SF-4 — Per-frame closure allocation in a panel that ticks while hidden

SocialAllegiancePageController.Tick() (src/AcDream.App/UI/Layout/SocialAllegiancePageController.cs:107-109):

IReadOnlyList<UiText.Line> lines = hasProfile ? NoLines : BlankLine;
if (_monarchName is not null) _monarchName.LinesProvider = () => lines;
if (_patronName is not null) _patronName.LinesProvider = () => lines;

() => lines captures a local, so this allocates a display class plus two delegates every frame, unconditionally, including while the panel is hidden (~180 allocations/second). NoLines and BlankLine are already static readonly; hoist two static readonly Func<> providers and pick between them. Cheap to fix, and Slice I's "0 B/resolve" discipline makes new per-frame garbage in a hidden panel worth catching now.

SF-5 — UiTemplateListBox.Flush()'s doc omits that it also resets scroll position, and silently diverges from the sibling Flush() it shares a name with

src/AcDream.App/UI/UiTemplateListBox.cs:239-252 documents only "resetting ContentHeight to 0". UiScrollablePanel.ClearContent() (src/AcDream.App/UI/UiScrollablePanel.cs:54-61) also calls Scroll.SetScrollY(0).

For a poll-and-rebuild list that is user-visible behavior: once MF-1 is fixed and the list can scroll, a user scrolled into a long Friends roster gets yanked to the top every time any friend's online status changes.

Worth noting explicitly because UiItemList.Flush() (src/AcDream.App/UI/UiItemList.cs:363-367) — the established sibling this method borrows its name from — does not touch scroll. Same name, different semantics, no doc distinguishing them.

SF-6 — #383's drift window is overstated; git narrows it to hours, not days

docs/ISSUES.md #383 says the two drifted fixtures were committed "(days ago, same machine)". Git says otherwise:

• keyboard_config_21000009.json — b4edee97, 2026-08-11 09:19
• options_2100002B.json — e71e5a96, 2026-08-11 06:25

The FA3 regeneration run was 2026-08-12 ~02:58 — i.e. ~18 h and ~21 h earlier, the previous day. That materially tightens the investigation window the issue asks for ("determine WHAT modified the installed DATs and when"): a same-day change is far easier to correlate against tooling activity than a multi-day one. Correct the parenthetical.

Everything else in #383 checks out against the evidence — see VERIFIED CLEAN 6.

SF-7 — The §10 addendum's bold markers are unbalanced

docs/research/2026-08-11-fa-panel-structure.md:900-916 contains five ** markers:

1. **[FA3 addendum, … is WRONG.** — opens and closes on the first sentence
2. **Allegiance** — balanced inline pair
3. the full citation.]** (line 916) — orphan, opens an emphasis run that never closes and bleeds into the following section

The FA2 convention the commit message cites (docs/research/2026-08-11-fa-acdream-seams.md:286-304) uses exactly two markers wrapping the whole block. Note the FA3 block also contains a fenced code block, which a markdown bold span cannot cross anyway — so the whole-block-bold intent needs a different treatment here (e.g. bold only the lead sentence and drop the trailing **).

---

NIT

N-8 — The gate script does not exercise the two things most likely to be wrong

docs/research/2026-08-12-campaign-fa-test-script.md has good coverage of the open paths, exclusivity, tab switching, empty states, and the closed-panel live update (§"Live update while the panel is closed" — nice catch, that is exactly the SF-2 path). It has no step for:

• scrolling a Friends/Squelch list longer than the visible area — precisely where MF-1 bites, and the user will not hit it with a short test roster;
• close the panel → log out → log back in, to confirm the restore-open behavior is wanted (see VERIFIED CLEAN 2).

Add both before the gate, otherwise MF-1 ships silently.

---

VERIFIED CLEAN

Stated plainly, because "no finding" here is itself the review product.

1. Catalog / window-name consumers fully enumerated, all degrade safely. OnWindowVisibilityChanged (RetailUiRuntime.cs:817) now resolves panel id 12 and calls ToolbarController.SetPanelOpen(12, …). That is a safe no-op: ToolbarController.cs:394-406 iterates _panelButtons and simply falls through when no button carries the id. SyncToolbarWindowButtons (:810) iterates ToolbarPanels only, where the social panel is correctly absent. RetailWindowOpacityController is scoped to the five chat windows by #379 (:152-158, :194-199) so it correctly ignores the new window. There is no UI-Studio consumer of these tables in the tree. TryGetPanelId / TryGetWindowName are linear scans over Mounted with no count assumptions anywhere.

2. Persistence is a deliberate, documented choice, not an oversight. WindowNames.SocialPanel is intentionally NOT in stateManagedVisibilityWindows (RetailUiRuntime.cs:440-446, whose members are Combat/JumpPowerbar/ExternalContainer/Vendor). The seam doc states the rule explicitly (2026-08-11-fa-acdream-seams.md: "if the panel should keep its own visibility across sessions, it must NOT be listed"), and the behavior matches its whole cohort — Options, Spellbook, Character, Inventory and Vitae all restore their own open state the same way. So the Configure-Keyboard restore-open surprise generalizes here, but as expected behavior consistent with every sibling, not a new defect. Flagged for the gate under N-8 only.

3. F3/F4 wiring introduces no conflict and no keybinds.json impact. The InputAction members, the RetailActionIdentityTable mappings (src/AcDream.UI.Abstractions/Input/RetailActionIdentityTable.cs:193-194) and the KeyBindings.RetailDefaults F3/F4 chords (KeyBindings.cs:209-210) all predate FA3 — git diff b560f415 d7e1cffd -- src/AcDream.UI.Abstractions/ is empty. FA3 adds only the handler, so the Configure Keyboard screen's conflict universe and display are unchanged; those two actions were already listed and rebindable, they just did nothing when pressed. I chased the apparent duplicate bare-F3 binding at KeyBindings.cs:119 (→ AcdreamDumpNearby): it lives in AcdreamCurrentDefaults() (:88-146), the WASD regression anchor that is not loaded in production, while RetailDefaults() (:147-401) deliberately relocates that action to Ctrl+F3 (:356). No conflict in the production table; F4 is unique there too. Nothing else consumed either action — ToolbarInputController.Handle (ToolbarInputController.cs:23-38) handles only quick-slot and CreateShortcut, so the new early-returns displace no prior behavior.

4. Mount order respects the documented constraint. MountSocialPanel is at RetailUiRuntime.cs:422, immediately after MountDialogFactory at :421 — satisfying the seam doc's rule (2026-08-11-fa-acdream-seams.md:548-552: "Any FA panel that raises a confirmation must be mounted after MountDialogFactory"). No existing panel's position in the ordered list moved, and RegisterMainPanel's duplicate-id/duplicate-name guard (RetailPanelUiController.cs:88-91) would have thrown had id 12 or "social-panel" collided.

5. Composition cannot silently degrade a host. SocialRuntimeBindings (RetailUiRuntime.cs:229-233) is a required positional member of RetailUiRuntimeBindings (:337), placed before the only optional member (Keyboard = null). There is exactly one construction site in the tree — InteractionRetainedUiComposition.cs:635 — and no test harness constructs the record. Any future host omitting it fails to compile rather than silently passing null.

6. The fixture generator is properly env-gated and the drift claim holds. RegenerateAllRetailFixtures_WhenExplicitlyRequested (RetailLayoutFixtureGenerator.cs:131-138) early-returns unless ACDREAM_REGENERATE_UI_FIXTURES=1, so a normal test run cannot rewrite any fixture. I diffed all four FA3 commits against b560f415: the only fixture touched is social_panel_2100006E_1000018F.json. The two drifted fixtures are genuinely excluded, exactly as the commit message and #383 claim.

7. Suite accounting reconciles exactly. 13 SocialPanelControllerTests + 1 SocialPanelLiveMountProbeTests + 4 RetailPanelCatalogTests = 18; 13,215 + 18 = 13,233. Confirmed by [Fact] counts per file and by running the targeted filter on the FA3-era Release binary (built 02:59, after both code commits):

   • ~SocialPanel|~RetailPanelCatalog → 23 passed, 0 failed (18 new + 5 pre-existing catalog tests).
   • Blast-radius regression filter ~TemplateListBox|~OptionsPage|~KeyboardConfig|~OptionsPanel|~WindowLayoutPersistence|~Toolbar → 289 passed, 0 failed. No regression on any shared surface FA3 touched.

8. UiTemplateListBox.Flush() is purely additive to existing consumers. No pre-existing consumer calls it; the four Options/Keyboard controllers are untouched. Dormancy is correctly preserved (_viewport?.ClearContent() never allocates the viewport). ClearContent is pre-existing and complete (clears children, _baseTops, ContentHeight, scroll). No interaction with #371 — that fix lives in LayoutScrollableChildren's intersection test (UiScrollablePanel.cs:77+), which Flush does not touch. Flush is also an established name across sibling list widgets (UiItemList, CreatureAppraisalRows, EffectsUiController, VendorUiController, SpellbookWindowController, InventoryController, ExternalContainerController), so the misuse surface is low — the only semantic gap is SF-5's scroll-reset divergence.

9. Register row AD-79 is well-formed. Six populated columns (row / what / files / why / risk-if-assumption-breaks / anchor), correctly scoped as ONE row covering all seven controls rather than one per button, and honest about covering both "not wired" and "not yet researched". The section header count is correctly incremented 58 → 59 active rows with AD-79 prepended to the chronological summary.

10. The §10 addendum is a clean, correctly-dated in-place correction. Pure insertion (+18 / −0) — it rewrites neither the original text nor any earlier addendum, and is dated 2026-08-12. Only its bold markup is off (SF-7).

11. I independently verified the headline correction from the committed fixture. The panel root's own authored TabTable reads:

    ButtonElementId	PageElementId	IsDefault
    0x1000028C	0x10000291	true
    0x1000028E	0x10000292	false
    0x10000512	0x10000513	false
    0x1000053B	0x1000054A	false

    That matches SocialPanelController's class doc and the §10 addendum exactly. Allegiance really is the authored default tab — lane A's x-order guess was wrong and the correction is sound.

12. No stale roster across reconnect — the FA2 MUST-FIX class does not recur. RuntimeCommunicationState.ResetFriends/ResetSquelch (src/AcDream.Runtime/Gameplay/RuntimeCommunicationState.cs:168-169, and the combined reset at :250-251) call Clear(), which increments Revision — so the panel's revision poll flushes the list on reset without any FA3-side reset plumbing.

13. The 0x10000492-authored-twice hazard is genuinely avoided. SocialAllegiancePageController.Bind scopes both name lookups to their own block container (:84-85, FindDescendant(monarchField, …) / FindDescendant(patronField, …)) rather than searching the page root — the campaign-OP 0x10000211-in-two-layouts lesson applied correctly.

---

Gate observation (not a finding)

Registering the social panel via RegisterMainPanel puts it in the shared main-panel geometry group, whose rectangle is cross-applied to every sibling (RetailPanelUiController.SynchronizeMainPanelSiblings → ApplyWindowGeometry). The social panel is authored 300x362; Options is 310x400. Height is therefore shared across differently-authored panels — but that is pre-existing, deliberate cohort behavior (the OP3 blast review's SHOULD-FIX 2 explicitly moved Options into this policy), not something FA3 introduced. Worth an eye at the gate: open Options first, then F3, and confirm the social panel is not visibly clipped or over-tall.