diff --git a/docs/research/2026-08-12-fa3-review-mechanism.md b/docs/research/2026-08-12-fa3-review-mechanism.md index 7f00933e..ba2b8d97 100644 --- a/docs/research/2026-08-12-fa3-review-mechanism.md +++ b/docs/research/2026-08-12-fa3-review-mechanism.md @@ -512,3 +512,144 @@ same fix round. Nothing here warrants a re-review of the mount path itself: the tab table, the frame swap, the string resolver, the scoped lookups, the tab activation, the catalog registration and the toolbar seam are all correct, and the live-mount probe passes against the installed DATs. + +--- + +# 4. NARROW RE-REVIEW — 2026-08-12 — **CLOSED, no reopen** + +Fix round `9afa05b5` / `35c40a9b` / `ae772709` / `a5553904` / `c5f73744`. +Every disposition below was re-derived from the actual diffs, not from the +commit messages. **All 2 MUST-FIX and all 9 SHOULD-FIX are correctly applied. +No finding is reopened. No new finding rises to MUST-FIX or SHOULD-FIX.** + +## 4.1 Mechanism dispositions + +| # | Verdict | Evidence | +|---|---|---| +| **MF-1** tab x-order in the gate script | **FIXED** | Steps 1, 9, 10 and the "What to report" bullet now all read **Allegiance, Fellowship, Friends, Squelch**, and step 1 states the point I asked for — "the default tab and the left-most tab are the SAME tab". Each correction carries an inline `[FA3 fix-round correction, mechanism MF-1]` note naming the earlier draft's error, matching the campaign's correct-in-place convention. | +| **MF-2** `@allegiance info` false-defect route | **FIXED, and inverted correctly** | Step 14 now states the true FA3 expectation in full: `@allegiance info` still prints real chat data, the panel blocks stay hidden all gate, "report NEITHER half as a bug". The "What to report" bullet is inverted from "blocks staying hidden" to "**blocks becoming VISIBLE at ANY point** … there is no code path in FA3 that can make that happen". That is the right anomaly to point the user at, and it cites the FA2 SHA (`4272ad0e`) for why. | +| **SF-1** unsupported retail claim on F3/F4 | **FIXED** | `SocialPanelController.cs:200-215` relabels it as acdream's own `OpenSpellbook`-precedent convention and reproduces all three negative findings (no `GetAttribute_Enum(this, 0x57, …)` site; `ClientUISystem::OnAction`'s `>0x7c` branch handles three unrelated ids; `gmPanelUI::ListenToGlobalMessage` COMDAT-folded onto a no-op). **U11 filed** in `fa-panel-structure.md` §8 with the same evidence and an honest "cdb trace is the only remaining way" recipe. I also checked the no-register-row argument rather than taking it: grepping the register for `Toggle*Panel` / close-on-second-press returns **nothing** — no row exists for the spellbook or any other non-toolbar toggle, so "following the existing no-row precedent" is factually true, not a convenient claim. | +| **SF-2** per-frame closure allocation | **FIXED** | Two `static readonly Func<…>` fields (`BlankLineProvider`/`NoLinesProvider`, `SocialAllegiancePageController.cs:59-67`); `Tick` now selects one and does a plain field write. Both lambdas close over `static readonly` fields only, so each is a single cached instance — the tick is genuinely zero-allocation now. | +| **SF-3** printed-but-unasserted probe findings | **FIXED, and better than asked** | `Assert.Equal(2, passupCount)` added; page exclusivity promoted to a real assertion that also pins **which** page (`Assert.Equal("Allegiance", visiblePageName)`) and fails loudly with both offending page names if two are visible. Re-ran live (§4.3): `Allegiance Visible=True`, other three `False`, `0x10000492 occurrences = 2`. | +| **SF-4** "three checkboxes" is four | **FIXED in both places** | `SocialFellowshipPageController.cs:19-21` now says FOUR and names `0x10000270`–`0x10000273`; gate-script step 11 drops the "a fourth may also be present" hedge. | +| **SF-5** AD-79 pins six of its seven controls | **FIXED** | `0x1000052C` added to the existing loop in `FriendsAndSquelchActionButtons_AreClickable_ButHaveNoHandler`, with a doc comment explaining why the checkbox slots into the button loop. Seven enumerated, seven pinned. | +| **SF-6** controllers don't cite AD-79 by id | **FIXED** | Both classes now read "See register row AD-79 (`docs/architecture/retail-divergence-register.md`)". Greppable in both directions. | +| **SF-7** allegiance gate coarser than retail's | **FIXED via the documented alternative** | Taken as the plan-line option I offered rather than a code change — correct call for a shell slice. The FA5 row now carries an explicit MUST: widen `Callbacks.AllegianceSnapshot` to reach `TryGetMonarch`/`TryGetPatron`, "a monarch character must NOT see an empty, visible monarch block". The class doc (`:37-49`) repeats it **and** carries the forward hazard I flagged — that FA5's name population must change `Tick`'s unconditional `LinesProvider` reassignment in the same commit. | +| **SF-8** `Tick()` ignores `_disposed` | **FIXED** | `if (_disposed) return;` is the first line of `SocialPanelController.Tick`. | +| **SF-9** `FindDeepest` doc overstates | **FIXED** | Rewritten to "the LAST `UiText` found by a pre-order walk", with the single-chain caveat and the live-dumped shape of both real templates. Describes what the code does. | + +## 4.2 Blast dispositions — spot-verified + +- **Scrollbar wiring (blast MF-1).** Both controllers resolve their authored + sibling scrollbar scoped to the page root and assign + `scrollbar.Model = listBox.Scroll`, with a log line on miss. Verified the + seam rather than assuming it: `UiTemplateListBox.Scroll => Viewport.Scroll` + (`:107`) forces the lazily-created viewport, which is already + extent-seeded by the #372 fix and returns one stable `UiScrollable`, so + wiring before the first row exists is sound. Type `0xB` really does build as + `UiScrollbar` (`DatWidgetFactory.cs:122`), the ids `0x10000518`/`0x10000543` + match each ListBox's own authored `ScrollbarElementId` (probe-confirmed), + and `scrollbar.Model = listBox.Scroll` is verbatim the shape + `CharacterOptionsPageController:323` / `ChatOptionsPageController:293` / + `ConfigOptionsPageController:397` already use. The four new tests pin both + the wiring (`Assert.Same`) and real reachability (40 rows exceeding the + authored extent, `HasOverflow`, scroll actually advances). +- **Template cache (blast SF-2).** `rowTemplateCache` memoizes the per-template + `ElementInfo?` (nulls included, so a permanent miss cannot re-walk the DAT) + while still `Build`-ing a fresh widget per row under the DAT lock. I checked + the soundness question this introduces — whether re-`Build`ing from one + shared `ElementInfo` is safe: `Build` is the documented pure builder and + mutates nothing; the only mutating entry point is `BuildFromInfos` + (`LayoutImporter.cs:68-78`, a tests-only convenience that overwrites + `rootInfo.Children`), which this path does not call. Safe. +- **Visibility-gated rebuild (blast SF-2).** `OnShown`/`OnHidden` are real + `IRetainedPanelController` members (`IRetainedPanelController.cs:12-16`) and + are genuinely driven — `RetailWindowHandle.NotifyVisibility` (`:92-108`) + fires them on every transition, and `AttachController`/`NotifyInitialState` + correctly do NOT fire `OnShown` for a window mounted `Visible = false`. So + `_visible` starts false in production exactly as the new test simulates. + Fellowship/Allegiance deliberately stay ungated (no DAT access) — right + split. `Friends_RevisionBumpWhileHidden_DoesNotRebuild_ButRebuildsOnShow` + pins both halves, including that the bump is deferred rather than lost. +- **Revision-latch ordering (blast SF-3).** `_lastRevision` now advances only + when every row resolved, so a transient resolver miss retries instead of + latching an empty list until the next server-side change. Checked the empty + -roster case: the loop body never runs, `allRowsResolved` stays true, the + revision latches — no spurious rebuild loop on an empty friends list. +- **`Flush` doc (blast SF-5).** Corrected to include the scroll reset, and it + goes further than the finding asked by naming the resulting UX cost + (a scrolled user yanked to the top on every rebuild), explaining why it was + NOT silently fixed, and flagging it forward. That is the right disposition + for an unasked behavior change. +- **#383 narrowing (blast SF-6).** I re-derived the timestamps rather than + trusting them: `keyboard_config_21000009.json` last written at `b4edee97` + (2026-08-11 09:19), `options_2100002B.json` at `e71e5a96` (2026-08-11 + 06:25), FA3's fixture commit `74c3d85d` at 2026-08-12 02:58 — ~17h39m and + ~20h33m earlier, the previous day. "~18h and ~21h, same day" is accurate, + and it does materially tighten the investigation window. The addendum + correctly attributes the new fixture's no-drift confirmation and narrows + the issue to the two OP-era fixtures. +- **Two new gate steps (blast N-8).** Step 19 (long-roster scroll) targets + exactly where the scrollbar fix bites and is honestly marked soft when the + roster is too short; its arithmetic is right (Friends box `270x400` inside a + `300x362` panel). Step 21 (restore-open across relaunch) is stated as + expected-not-a-bug, and its premise checks out — `stateManagedVisibilityWindows` + is still `[Combat, JumpPowerbar, ExternalContainer, Vendor]` + (`RetailUiRuntime.cs:440-446`), so the social panel restores like every + sibling main panel. + +## 4.3 Gates re-run on the post-fix binaries + +Release binaries timestamp 2026-08-12 03:44, i.e. at/after the last code +commit (`ae772709`, 03:44) — the runs below are against the fixed code. + +- **Live-mount probe** (`ACDREAM_PROBE_LIVE_MOUNT=1`, installed DATs): + **1 passed / 0 failed.** The two promoted assertions pass with observable + values — `page Allegiance … Visible=True`, Friends/Fellowship/Squelch + `Visible=False`, `0x10000492 occurrences under allegiance page = 2` — and + all four captions still resolve (`Allegiance`/`Fellowship`/`Friends`/`Squelch`). +- **`AcDream.App.Tests`, Release, `--no-build`: 4,876 passed / 3 skipped / + 0 failed**, exactly `+5` over the 4,871 I measured pre-fix-round and exactly + the ledger's own per-project App figure. The `+5` reconciles: five genuinely + new `[Fact]`s (two scrollbar-wiring, two long-roster reachability, one + hidden-vs-shown rebuild), with the two extended-in-place tests and the two + new probe assertions correctly counted as net zero. The 13,238/4/0 solution + total is therefore consistent at the only project this fix round touched; I + did not re-run the other eight (no builds permitted). +- **Blast radius**: `git diff --stat 9e622f56..c5f73744` touches 14 files, all + FA3's own plus doc-only edits to the shared `UiTemplateListBox` and the + `MountSocialPanel` method. No collateral edits. + +## 4.4 Carry-forward (non-blocking, for FA4/FA5 — not reopened findings) + +1. **Scroll position resets on every rebuild.** Correctly deferred and + documented in `UiTemplateListBox.Flush`. It bites harder in FA4, whose + fellow roster rebuilds on every vitals tick rather than on a rare roster + change — worth resolving there rather than deferring again. +2. **The production template cache is untested.** Both long-roster tests use + `FakeRowTemplateResolver`, so `rowTemplateCache` itself has no coverage. I + verified its soundness by inspection (§4.2), but a rebuild-twice assertion + in the live-mount probe would cost one line and cover the real path. +3. **Hardcoded scrollbar ids.** `0x10000518`/`0x10000543` are literals when + `UiTemplateListBox.ScrollbarElementId` already carries the authored value. + Both match today (probe-confirmed) and this mirrors the existing Options-tab + consumers, so it is a cohort-wide nit, not an FA3 defect. +4. **`allRowsResolved` retry loop.** A permanently unresolvable template makes + the list rebuild every frame while the panel is open. Bounded and cheap + (the cache stores the null, so no DAT walk; `Build` is skipped entirely) — + recorded so it is not rediscovered as a mystery. + +## 4.5 Verdict + +**CLOSED — no reopen.** Every mechanism finding is applied, and each was +applied at the right altitude: the two MUST-FIX as script rewrites that state +the verified truth rather than merely deleting the wrong claim, SF-7 as a +binding FA5 acceptance line rather than scope creep into a shell slice, and +SF-3 as assertions that pin more than the finding asked for. The fix round +also left the two things it chose not to change (the Flush scroll reset, the +coarser allegiance gate) documented and forward-flagged rather than silent, +which is the disposition this project's register rules ask for. FA3's +remaining obligation is unchanged: the user's connected gate, now against a +script that no longer contains two instructions guaranteed to produce false +defect reports. +