# 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 = .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`): ```csharp IReadOnlyList 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.