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:

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