acdream/docs/research/2026-08-11-op5-recheck.md

# Campaign OP slice OP5 — fix-round re-check

**Commit re-checked:** `6d0b0f92` "fix(ui): OP5 review fixes — thumb sync, batched
opacity writes, cull register row, tests"
**Against:** `docs/research/2026-08-11-op5-review.md` (M1 MUST-FIX, S1/S2/S3
SHOULD-FIX, N11)
**Mode:** read-only (no build, no test run, no launch); git reads only
**Verdict: CLOSED** — all six findings genuinely closed. 0 MUST-FIX, 2 SHOULD-FIX
and 5 NOTE residuals, all on the NEW surface the fix introduced rather than on
the findings themselves.

---

## Per-finding closure

### M1 — CLOSED

`src/AcDream.App/UI/Layout/ChatOptionsPageController.cs:361-382`

Both apply closures now push their own slider first, from the post-link truth:

```csharp
bindings.SetDefaultOpacity(value);
slider1.SetScalarPosition(bindings.CurrentDefaultOpacity()); // M1: own slider first
activeRow!.RefreshFromLink(bindings.CurrentActiveOpacity());
```

symmetric at `:376-378` for `slider2`. This is the sibling pattern the review
cited (`CharacterOptionsPageController.cs:379-385`, and this file's own filter
block).

**Exact scenario walked** (drag Default only → Reset → value AND thumb revert):

1. Seed default 0.3 / active 0.9. `slider1.ScalarPosition = 0.3`,
   `defaultRow._saved = 0.3`.
2. Drag Default down to 0.1. `UiScrollbar.ChangeScalarPosition`
   (`UiScrollbar.cs:566-570`) → `ScalarChanged` → `FloatOptionRow.SetCurrentValue`
   (`OptionPageModel.cs:234-239`) → `apply`. `ChatOpacityLink.SetDefault(0.9, 0.1)`
   leaves active at 0.9 (`ChatOpacityLink.cs:31-37`), so `activeRow.Changed`
   stays false and only `defaultRow` is changed — the review's exact
   single-row case.
3. Click Reset → `OptionPage.Reset()` (`OptionPageModel.cs:628-633`) restores
   only `defaultRow` → `RestoreSavedValue` (`OptionPageModel.cs:269-273`) calls
   `_apply` and never `_refresh` — unchanged. The revert now reaches the thumb
   because `_apply` itself writes `slider1.SetScalarPosition(0.3)`.

**Do the tests assert a reachable thumb state?** Yes.
`SetScalarPosition` writes `ScalarPosition` (`UiScrollbar.cs:85-86`), which is
the exact value the draw path multiplies into the thumb origin
(`UiScrollbar.cs:228-230`, `float x = travel * ScalarPosition`). Asserting
`ScalarPosition` IS asserting thumb position; there is no separate cached
geometry.
`Reset_AfterOnlyDefaultSliderChanged_RevertsTheThumbToo` and its Active twin
(`tests/AcDream.App.Tests/UI/Layout/ChatOptionsPageControllerTests.cs:472-535`)
assert it, and both also assert `Assert.False(otherRow.Changed)` so the
self-healing two-row case cannot be mistaken for the single-row one.

**Does drag-the-other survive (AP-190 never-clamp)?** Yes, untouched. The link
returns the value being set verbatim — `SetDefault` returns `newDefault`
(`ChatOpacityLink.cs:36`) and `SetActive` returns `newActive` (`:50`); only the
OTHER value moves. `RetailWindowOpacityController.SetDefaultOpacity/SetActiveOpacity`
(`RetailWindowOpacityController.cs:89-105`) delegate straight to it. So
`bindings.CurrentDefaultOpacity()` inside `defaultRow`'s own apply is always
exactly the dragged value (clamped to [0,1] only), and the new own-slider push
is a no-op mid-drag and load-bearing only on Reset/Defaults. It cannot fight a
drag or re-introduce clamping.

### S1 — CLOSED

**Fires exactly once per real drag, all three paths.** The identical
`bool wasDragging` guard is applied at all three `MouseUp` sites — vertical
model `UiScrollbar.cs:443-450`, horizontal model `:500-507`, horizontal scalar
`:543-550`. `_draggingThumb` is set only inside a `MouseDown` branch and
cleared only at `MouseUp`, so within one real gesture exactly one
`DragCompleted` is emitted. Covered by four tests
(`tests/AcDream.App.Tests/UI/UiScrollbarTests.cs:106-201`), including
"10 MouseMoves fire zero completions".

**The `IsDragging` discriminator.** The claim "a slider drag and a button click
can never be concurrent" holds under this input model, for a stronger reason
than the phrasing suggests: the pressed slider holds pointer capture
(`UiScrollbar()` sets `CapturesPointerDrag = true`, `UiScrollbar.cs:152`;
`UiRoot.OnMouseDown` → `SetCapture(target)`, `UiRoot.cs:492`, then the
`CapturesPointerDrag || HandlesClick` branch at `:549-557`), and Reset/Defaults
are `UiButton` clicks that require their own left-button MouseDown → left-up
over the same element (`UiRoot.cs:649-661`). One left button cannot be down on
both. Also note the discriminator is correctly per-slider: `defaultRow`'s apply
can only be reached from `slider1`'s own `ScalarChanged` or from Reset/Defaults —
dragging `slider2` reaches `defaultRow` via `RefreshFromLink`, which never
calls apply (`OptionPageModel.cs:250-255`).

**Live opacity still applies per tick.** `SetDefaultOpacity`/`SetActiveOpacity`
still run `ReapplyAll()` on every call (`RetailWindowOpacityController.cs:93,104`);
only the disk write moved. Pinned by
`DraggingDefaultSlider_DefersTheSettingsWriteUntilDragEnd`'s
`Assert.NotEqual(0.2f, fakeBindings.DefaultOpacity)`
(`ChatOptionsPageControllerTests.cs:625` region).

**Reset/Defaults flush immediately.** Neither is mid-drag, so
`if (!sliderN.IsDragging) bindings.FlushOpacity()` fires inline — same
single-write-per-discrete-edit shape as pre-fix. Pinned by
`ResetClick_FlushesImmediately_NotMidDrag` (2 flushes: the edit, then the
Reset). `Defaults()` restores both rows unconditionally
(`OptionPageModel.cs:638-643`) → 2 flushes, same as pre-fix.

**No save regression on other paths.** `SaveChatOpacity` had exactly two callers
before this commit (`git grep SaveChatOpacity 6d0b0f92^` → `RetailUiRuntime.cs:2126,2131`,
the two setter lambdas); both are replaced by the single `FlushOpacity:
SaveChatOpacity` wiring (`RetailUiRuntime.cs:2136`). Nothing else called it.
The other opacity consumer, `RuntimeSettingsTargets.cs:223`, goes through
`RetailWindowOpacityController.SetOpacity` and owns its own persistence — never
touched `SaveChatOpacity` and is unaffected.

**Page hide.** Covered, indirectly but correctly. `OnHidden() => Reset()`
(`OptionPageModel.cs:672`) restores every changed row through `apply`, which
flushes (not mid-drag). There is no state where a hide can strand an unflushed
value: the only value the disk lags is a mid-drag one, and a mid-drag hide
reverts live opacity to the saved value the disk already holds. See
SHOULD-FIX R1 below for what a mid-drag hide *does* break.

### S2 — CLOSED, and the correction is honest

- **AP-201** is filed and its text is accurate: it states the cull predates OP5
  ("Filed 2026-08-11 at the OP5 review-fix round (S2), predates OP5 but was made
  user-visible by it"), quotes the cull expression verbatim, and names the
  260 px blocks as the reason it is newly visible.
- **#371** (`docs/ISSUES.md:27-47`) matches, cites
  `src/AcDream.App/UI/UiScrollablePanel.cs:69`, states the root-cause fix
  (a real per-row clip rect) and explicitly records that it was NOT attempted.
- The cited line is still exactly what the register quotes
  (`UiScrollablePanel.cs:69`: `child.Visible = top >= -0.5f && top + child.Height <= Height + 0.5f;`),
  and the class doc at `:8-12` still says "clips whole rows because the UI
  renderer does not have a scissor stack yet".
- **Gate-script correction states the expected vanish-reappear honestly**
  (`docs/research/2026-08-11-campaign-op-test-script.md`, step 2 and the
  "report as a bug" list): "can vanish ENTIRELY at that exact offset rather than
  showing a partially-clipped view. Keep scrolling a little further and the block
  reappears whole." The old "any block whose height looks clipped" bullet was
  rewritten rather than deleted — it still catches the genuine self-sizing
  regression ("STILL missing rows, or leaves a large empty gap … once fully
  scrolled into view"). Nothing was hidden from the gate.

### S3 — the review-correction claim is TRUE; the new test is partly real

`git grep ChatWindowMainFilter e71e5a96 -- tests/.../SettingsStoreTests.cs`
returns `:340` (default assertion) and `:347-352` (an explicit
`original`→`SaveChat`→`LoadChat`→`Assert.Equal` round trip with
`ChatWindowMainFilter = 0x1ul`). The round-trip test **did** exist at
`e71e5a96`; the review's S3 first half was wrong and the commit's correction is
verified.

The replacement coverage (`tests/AcDream.App.Tests/UI/ChatMainWindowFilterSeedTests.cs`)
pins something real, but less than its own doc-comment implies — see NOTE R5.

### N11 — CLOSED as asked; residual is inherent

`ChatOptionsPageControllerTests.cs:770-793` now resolves both the ListBox and
the scrollbar through `UiElement.FindDescendant(chatSlot, …)` from the page slot
`0x1000050C`, matching `ConfigOptionsPageControllerTests.ScrollbarLinkage_ModelPointsAtTheConfigListBoxScroll`
(`:499-505`). That is exactly the shape N11 requested, and the production
lookup it mirrors is unchanged and correct
(`ChatOptionsPageController.cs:236-246`). The "asserts on the wrong object"
half of N11 is closed. The "would not catch a de-scoping regression" half is
only partly closed — see NOTE R4.

---

## Blast radius of the NEW surface

**`UiScrollbar.IsDragging` / `DragCompleted` — no effect on other consumers.**
`DragCompleted` defaults to `null` and has exactly two writers, both in
`ChatOptionsPageController.cs:388-389`; `IsDragging` is a read-only projection
of the existing `_draggingThumb` field (`UiScrollbar.cs:44`) with two
non-production readers besides the two apply closures (tests). The three
`MouseUp` bodies are otherwise byte-equivalent to before: both fields are still
cleared and the handler still returns `true`. Every other scrollbar in the
codebase (chat transcript, item lists, favorite-spell bar, combat power meter)
is behaviorally identical.

**`Bindings` record growth (`FlushOpacity` inserted mid-list) — all sites
updated.** `ChatOptionsPageController.Bindings` is a positional record, but all
three construction sites use named arguments, so a positional shift is
impossible:
`src/AcDream.App/UI/RetailUiRuntime.cs:2129-2144` (production),
`tests/AcDream.App.Tests/UI/Layout/ChatOptionsPageControllerTests.cs:169-186`
(`FakeBindings.ToBindings`), and
`tests/AcDream.App.Tests/UI/Layout/ConfigOptionsPageControllerTests.cs:558-570`
(the OP6 test file's local Chat fake, updated in this commit). Grep for
`ChatOptionsPageController.Bindings(` finds no fourth site.

**`RetailUiRuntime` save wiring — no path lost its save.** Covered under S1
above: two pre-change callers, both accounted for; no other caller existed.

---

## Residuals

### SHOULD-FIX

**R1 — a capture loss mid-drag strands `_draggingThumb = true`, and the new
discriminator turns that stale flag into a silent persistence failure.**

`_draggingThumb` is cleared in exactly one place per path — the `MouseUp` case
(`UiScrollbar.cs:446`, `:503`, `:546`) — plus the `IsModelDisabled` early-out
(`:362-368`, unreachable for a scalar slider because `ScalarChanged` is
non-null). But `UiRoot` can drop pointer capture WITHOUT delivering a `MouseUp`:

- `UiRoot.ClearSubtreeOwnership` → `ReleaseCapture()` (`UiRoot.cs:263-271`),
  reached from `OnElementVisibilityChanging` (`UiRoot.cs:253-258`), which the
  `UiElement.Visible` setter calls on every hide (`UiElement.cs:124-137`).
  Reachable by keyboard while the button is held: `ToggleOptionsPanel()` →
  `ToggleWindow(WindowNames.Options)` (`src/AcDream.App/Input/GameplayInputCommandController.cs:40-41`).
- A second `MouseDown` (right/middle button) on a different element re-targets
  capture unconditionally (`UiRoot.cs:492`), so the subsequent left-up is
  delivered to the new target and never reaches the slider.

After either, `IsDragging` reads `true` forever for that slider, so
`if (!slider1.IsDragging) bindings.FlushOpacity()` (`ChatOptionsPageController.cs:366,379`)
never fires again for Reset/Defaults/discrete edits on that row: the value
applies live but is never persisted, with no log. Drags still flush (the next
real `MouseUp` sees `wasDragging == true`), which is what makes it silent.

Pre-fix, the same stale flag was benign (a stuck `_draggingThumb` with no
capture receives no `MouseMove`); this commit is what gives it a consequence.
The seam for the root-cause fix already exists: `UiRoot.PointerCaptureChanged`
(`UiRoot.cs:213`, raised at `:790` and `:801`) — or `IsDragging` could be
defined as "dragging AND still the captured element".

**R2 — the `DragCompleted` doc claim is false on the scalar path, and the
"never a drag" test does not cover the case that breaks it.**

`UiScrollbar.cs:47-50` promises the callback fires "never on a `MouseUp` that
was not preceded by an actual drag (a bare click that only page-scrolled or
**jumped**…)". True for the two model paths (`_draggingThumb` is set only inside
the thumb-hit branch, `:412-417` and `:473-477`), but false for the scalar path
the opacity sliders use: `OnScalarEvent`'s `MouseDown` sets
`_draggingThumb = true` **unconditionally** at `UiScrollbar.cs:531`, after the
jump branch at `:527-530`. So a bare click on the opacity track fires
`DragCompleted`.

Second-order: that same click produces **two** flushes, because
`ChangeScalarPosition` at `:529` runs while `_draggingThumb` is still `false`
(the flag is set on the next line), so the apply closure flushes inline, and
then the `MouseUp` flushes again. Harmless (idempotent whole-file write, 2 not
dozens), but it contradicts "exactly one per gesture" in the same doc block.

`HorizontalScalar_DragCompleted_DoesNotFireOnAMouseUpThatWasNeverADrag`
(`UiScrollbarTests.cs:139-155`) only covers a `MouseUp` with no prior
`MouseDown` at all, so nothing catches this. Either correct the doc (and add a
track-jump-click test asserting the real behavior) or gate `_draggingThumb` on
the thumb-hit branch in `OnScalarEvent` the way the other two paths do.

### NOTE

**R3 — the M1 tests do not drive the real drag pipeline, so the assertion that
genuinely discriminates is the intermediate one, not the Reset one.**
`Reset_AfterOnlyDefaultSliderChanged_RevertsTheThumbToo` moves the value with
`defaultRow.SetCurrentValue(0.1f)` rather than through `UiScrollbar` events. On
pre-fix code `slider1.ScalarPosition` would have stayed at its 0.3 seed, so the
post-Reset assertion (`Assert.Equal(0.3f, slider1.ScalarPosition)`) would pass
by accident; the test fails pre-fix at the earlier
`Assert.Equal(0.1f, slider1.ScalarPosition, 3)` instead. It does discriminate,
just not at the line that reads like the point of the test. The S1 test
(`DraggingDefaultSlider_…`) does drive the real `UiScrollbar` event pipeline;
folding a Reset assertion onto the end of that one would pin the review's
literal scenario.

**R4 — N11's "would not catch a de-scoping regression" is still open for the
Chat side, for the reason N11 itself identified.** `ImportedLayout`'s flat map
is last-write-wins and the Chat slot is built last, so
`layout.FindElement(0x10000201)` and the scoped lookup resolve the SAME
instance today. A Chat-side regression to a flat lookup therefore still passes
both the reworked test and OP6's
`SharedScrollbarId_ChatAndConfigBoundTogether_EachOwnsItsOwnScrollbar`
(`ConfigOptionsPageControllerTests.cs:508-554`) — that test discriminates the
**Config** side only (Config binds second; a flat Config lookup would clobber
Chat's `Model` and trip `Assert.Same(chatListBox.Scroll, chatScrollbar.Model)`).
The reworked assertion is still the right shape — it starts failing the moment
build order changes — but no test can fail on a Chat de-scoping while the
coincidence holds. Not fixable without changing build order; worth one line in
the test's own comment so a future reader does not over-trust it.

**R5 — the new seed tests re-implement the production statement instead of
invoking it.** `ChatMainWindowFilterSeedTests.cs:47` and `:58` copy the two
lines from `RetailUiRuntime.MountChat` (`RetailUiRuntime.cs:947-949`) verbatim
into the test body, so neither test can fail if that seed is deleted or
re-pointed at the wrong window id — the thing S3 asked to pin. What they DO pin
that nothing else did: the cross-type default agreement
(`ChatSettings.Default.ChatWindowMainFilter` = `0xFBFFFFFF`,
`ChatSettings.cs:71`, equals `ChatWindowState.MainWindowDefaultFilter`), i.e.
the seed is a faithful no-op on a fresh install. Test 1 is largely redundant
with the pre-existing `SettingsStoreTests` round trip. The commit message is
honest about the limitation; the test's own doc comment ("pins the two-line
seed") reads stronger than what it does.

**R6 — doc wording: "the link may have clamped it".**
`ChatOptionsPageController.cs:304-305` explains the post-link read as "not the
raw dragged value — the link may have clamped it". The link never clamps the
value being set — that is precisely AP-190 / `ChatOpacityLink`'s stated
invariant ("enforced by dragging the OTHER value, never by clamping the one
being set", `ChatOpacityLink.cs:4-6`); the only clamp is the [0,1] range clamp.
The comment as written invites a future reader to believe the opposite of the
invariant. "the link normalizes it to [0,1]" would be accurate.

**R7 — the Chat tab's Apply button does not flush opacity.** `AfterApply` for
all three option pages publishes `SaveCharacterOptionsRuntimeCmd`
(`RetailUiRuntime.cs:2044`, `OptionsPanelController.cs:150-154`), which has
nothing to do with the local `settings.json` opacity blob. Harmless — Apply
changes no value, it only commits baselines, and every value-changing path
flushes — but it means the flush set really is exactly {drag end, discrete
edit, Reset, Defaults}, as the S1 doc says. Recorded so a future reader does
not assume Apply is a backstop.

---

## Summary table

| ID | Class | One-line |
|---|---|---|
| M1 | CLOSED | Both apply closures push their own slider from the post-link truth; drag-Default-only → Reset now reverts the thumb; never-clamp untouched |
| S1 | CLOSED | One flush per gesture across all three drag paths; Reset/Defaults/hide flush inline; live apply still per tick; no other save path affected |
| S2 | CLOSED | AP-201 + #371 accurately scope the cull as pre-existing/made-visible; gate script names the vanish-reappear honestly and keeps the real regression bullet |
| S3 | CLOSED | Round-trip test verified present at `e71e5a96` — the review was wrong; replacement pins the default agreement (see R5) |
| N11 | CLOSED | Scoped page-slot assertion landed, matching OP6's pattern (residual R4) |
| — | Blast radius | Clean: `DragCompleted` null-default with 2 writers, `MouseUp` otherwise unchanged; 3 `Bindings` sites all named-arg; 2 pre-change `SaveChatOpacity` callers both accounted for |
| R1 | SHOULD-FIX | Capture loss without `MouseUp` strands `_draggingThumb`; stale `IsDragging` silently disables the Reset/Defaults flush for the session |
| R2 | SHOULD-FIX | `DragCompleted` doc contradicts `OnScalarEvent` (a bare track-jump click IS a "drag"); that click double-flushes; untested |
| R3 | NOTE | M1 tests bypass the real drag pipeline; the discriminating assertion is the intermediate one |
| R4 | NOTE | Chat-side de-scoping still undetectable (flat winner == Chat instance); only the Config side is discriminated |
| R5 | NOTE | Seed tests copy the production statement rather than invoking it; what they really pin is the default agreement |
| R6 | NOTE | "the link may have clamped it" contradicts the never-clamp invariant it cites |
| R7 | NOTE | Chat-tab Apply publishes character options only — not an opacity flush backstop |