diff --git a/docs/research/2026-08-12-fa3-review-blast.md b/docs/research/2026-08-12-fa3-review-blast.md new file mode 100644 index 00000000..81e36a96 --- /dev/null +++ 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 = .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.