docs: FA3 blast-radius review -- APPROVE-WITH-FIXES (1 MUST-FIX, 6 SHOULD-FIX)

MUST-FIX 1: the Friends/Squelch lists have NO scroll driver. Both controllers set TemplateResolver but never wire the authored scrollbar's Model (0x10000518 / 0x10000543, both direct siblings of their ListBox under the page root per FA3's own fixture), unlike all four existing UiTemplateListBox consumers. There is no wheel fallback -- wheel scroll lives only on UiText, not UiScrollablePanel -- so Scroll has no driver at all. Visible at authored size: the 400/430-tall ListBoxes sit at y=40 in a 362-tall panel, so rows past ~322px are off-panel AND unreachable. AD-79 covers the inert BUTTONS, not a dead scrollbar. SHOULD-FIX: the revision-driven rebuild does N live DAT imports under the shared DatLock while the panel is CLOSED (first repeating consumer of a resolver every other caller invokes once at Bind); Refresh() consumes the revision before building so one resolver miss latches an empty list; per-frame closure allocation in the hidden allegiance page; Flush()'s doc omits its scroll reset and diverges from the sibling UiItemList.Flush() it shares a name with; #383's "days ago" is really ~18-21h per git; the SS10 addendum's trailing ** is orphaned. Verified clean: catalog/window-name consumers all degrade safely (SetPanelOpen(12) is a no-op, opacity controller correctly scoped by #379); persistence omission is the documented cohort behavior; F3/F4 plumbing predates FA3 entirely (UI.Abstractions untouched) and the apparent bare-F3 duplicate is in the non-production AcdreamCurrentDefaults; mount order respects the DialogFactory constraint; SocialRuntimeBindings is required-positional with one construction site; the generator is env-gated and only the new fixture landed; +18 reconciles exactly (23 targeted + 289 blast-radius regression tests pass on the FA3 binary); AD-79 well-formed with the count bumped 58->59; and the corrected tab table was independently re-read from the fixture's own TabTable (0x1000028C -> 0x10000291, IsDefault=true -- Allegiance IS the default). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-08-12 03:14:42 +02:00 · 2026-08-12 03:14:42 +02:00 · e9eb756480
commit e9eb756480
parent 3e6dc62b0e
1 changed files with 338 additions and 0 deletions
--- a/docs/research/2026-08-12-fa3-review-blast.md
+++ b/docs/research/2026-08-12-fa3-review-blast.md
@ -0,0 +1,338 @@
+# 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`):
+
+```csharp
+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.