Conversation
Use non-animated ScrollView extent navigation instead of retaining an ItemsView bring target across history replacement. Workaround for microsoft/microsoft-ui-xaml#11865; related to openclaw#1167. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 12, 2026, 2:32 PM ET / 18:32 UTC. ClawSweeper reviewWhat this changesReplace chat’s automatic row-targeted scrolling with non-animated scrolling to the bottom, and update the regression guard and validation documentation. Merge readiness⛔ Blocked before merge - 4 items remain Keep open: the workaround remains useful and no concrete patch defect was found, but the reported single successful session selection does not establish recovery under repeated switching. Priority: P1 Review scores
Verification
How this fits togetherThe native chat timeline receives session histories and streaming messages from the chat provider. Its scroll controller positions newly loaded histories and follows new messages while respecting readers who scroll away. flowchart TD
A[Session history loaded] --> C[Chat timeline]
B[New messages] --> C
C --> D[Validate current tail and generation]
D --> E{Initial positioning or following?}
E -->|Yes| F[Scroll to bottom extent]
E -->|No| G[Preserve reader position]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the workaround within the existing scroll owner, preserving native virtualization, bottom following, and scroll-away intent while tracking the sustained hang separately. Do we have a high-confidence way to reproduce the issue? No high-confidence current-main application reproduction was independently established. The upstream minimal reproducer, native stacks, and unchanged main call path provide a concrete source-supported investigation path. Is this the best way to solve the issue? Yes at the code-boundary level: avoiding the retained row target is a narrow workaround for the documented native failure without replacing virtualization or weakening lifecycle guards. Runtime coverage remains incomplete. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against bd9ce43b4d9c. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Summary
This is an application-side workaround for microsoft/microsoft-ui-xaml#11865, not a fix in WinUI.
Repeated chat-session switching can recycle a row retained by
ItemsView.StartBringItemIntoView. A later native anchor request then rejects that row withE_INVALIDARG, terminating the app. The upstream issue includes the minimal reproducer and native failure details.Replace automatic tail navigation with non-animated
ScrollView.ScrollTo, using the current horizontal offset and scrollable height. This avoids the retained bring-into-view target. Keep the existing tail identity, generation, loaded-state, queue, and bottom-follow guards unchanged. Add a source regression guard against reintroducing the affected API and document the runtime proof path.Ownership remains in
InitialTailPositionerinReactorItemsViewScrollController.cs. There is no new realization logic, layout loop, timer, or exception suppression.Related: #1167. This targets the confirmed crash; it does not establish that the separately reported sustained UI-thread hang is resolved.
Required proof pools
windows-11-arm64: native ARM64 build and runtime validation on the architecture where the crash was investigated.windows-winui-interactive: chat session navigation and visible timeline behavior.Validation
.\build.ps1: passed, including repository documentation checks.dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore: 3,998 passed, 34 skipped, 0 failed.dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore: 2,976 passed, 0 failed.python .agents\skills\autoreview\scripts\autoreview --mode localexited 9009 because Python is not installed; Codex is also unavailable on PATH. No clean Codex review is claimed.Real behavior proof
The developer confirmed the original ScrollView workaround worked. The PR preserves that small workaround.
The final ARM64 Debug build was also launched with isolated tray settings against a loopback-only synthetic gateway serving two 77-message, mixed-height histories. After selecting Workaround two, the app remained responsive, the composer was visible, and an inspected screenshot showed the final row completely:
The locally built WinApp CLI was used for UI automation, screenshots, ETW recording, and
perf analyze. No private chat content, credentials, dumps, or raw traces are published.Not verified / blocked