Skip to content

fix(chat): work around WinUI session-switch crash - #1407

Open
dotMorten wants to merge 1 commit into
openclaw:mainfrom
dotMorten:pr/1167
Open

dotMorten wants to merge 1 commit into
openclaw:mainfrom
dotMorten:pr/1167

Conversation

@dotMorten

Copy link
Copy Markdown

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 with E_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 InitialTailPositioner in ReactorItemsViewScrollController.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.
  • Rubber-duck review of the final diff and controller lifecycle: no actionable findings.
  • Structured autoreview: blocked. python .agents\skills\autoreview\scripts\autoreview --mode local exited 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:

Session: Workaround two
Workaround two: message 77
FINAL MESSAGE: Workaround two

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

  • The extended automated switching loop did not complete: its session-row selector failed after one successful selection. This is not a passing 20-switch stress test.
  • ETW analysis returned partial evidence: 0 lost events/buffers, 1 decode error, and 51 incomplete scope boundaries. No timing or performance conclusion is claimed.
  • Streaming arrivals while scrolled up, tail positioning across every history shape, and the original sustained hang were not independently established by this closeout.

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>
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 12, 2026
@clawsweeper

clawsweeper Bot commented Sep 12, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 12, 2026, 2:32 PM ET / 18:32 UTC.

ClawSweeper review

What this changes

Replace 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
Reviewed head: 1091062a0987dafe940cea5c2baf6049124b410d

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, plausible workaround with positive native evidence, but incomplete coverage of its repeated-switch trigger.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: The supplied ARM64 native-app report exercises InitialTailPositioner through one session selection and records a visible final row, which is useful after-fix evidence. Its explicitly incomplete switching loop does not cover repeated row replacement, and scroll-away behavior during arrivals remains unverified. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The supplied ARM64 native-app report exercises InitialTailPositioner through one session selection and records a visible final row, which is useful after-fix evidence. Its explicitly incomplete switching loop does not cover repeated row replacement, and scroll-away behavior during arrivals remains unverified. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 9 items Introduced change and preserved guards: The pinned three-file patch changes only the native navigation operation, its source regression assertions, and documentation. Loaded-state, tail identity, generation, queue, disposal, and follow guards remain intact; the timeline supplies its final displayed row.
Still needed on main and latest release: Inspection of the fetched main revision and v2026.9.3 confirms both retain StartBringItemIntoView in StartTailRequest. The workaround is not present in either inspected version.
Dependency boundary: The application directly uses Microsoft.UI.Xaml controls and pins Microsoft.WindowsAppSDK 2.4.0, matching the version identified by the upstream reproducer. The WinUI anchor contract is therefore relevant to this patch.
Findings None None.
Security None None.

How this fits together

The 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]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The supplied ARM64 native-app report exercises InitialTailPositioner through one session selection and records a visible final row, which is useful after-fix evidence. Its explicitly incomplete switching loop does not cover repeated row replacement, and scroll-away behavior during arrivals remains unverified. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - Repeated history replacement and final-row positioning after changing native navigation APIs remain incompletely demonstrated.
  • Resolve merge risk (P1) - The separate sustained UI-thread hang remains unresolved by the available evidence.
  • Complete next step (P2) - Add current-head native evidence for repeated session switching, final-message visibility, and new arrivals while scrolled up. A recording is preferred; copied diagnostics or redacted logs also count. Redact private information before posting. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +8/-6; tests +6/-3; docs +1/-0 The narrowly scoped workaround adds no new layout owner, retry loop, or configuration surface.

Merge-risk options

Maintainer options:

  1. Decide the mitigation before merge
    Keep the workaround within the existing scroll owner, preserving native virtualization, bottom following, and scroll-away intent while tracking the sustained hang separately.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Technical review

Best 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.

Labels

Label changes:

  • add P1: The workaround addresses a reported native application crash during ordinary chat-session switching.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The supplied ARM64 native-app report exercises InitialTailPositioner through one session selection and records a visible final row, which is useful after-fix evidence. Its explicitly incomplete switching loop does not cover repeated row replacement, and scroll-away behavior during arrivals remains unverified. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P1: The workaround addresses a reported native application crash during ordinary chat-session switching.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The supplied ARM64 native-app report exercises InitialTailPositioner through one session selection and records a visible final row, which is useful after-fix evidence. Its explicitly incomplete switching loop does not cover repeated row replacement, and scroll-away behavior during arrivals remains unverified. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Introduced change and preserved guards: The pinned three-file patch changes only the native navigation operation, its source regression assertions, and documentation. Loaded-state, tail identity, generation, queue, disposal, and follow guards remain intact; the timeline supplies its final displayed row. (src/OpenClaw.Tray.WinUI/Chat/ReactorItemsViewScrollController.cs:255, 1091062a0987)
  • Still needed on main and latest release: Inspection of the fetched main revision and v2026.9.3 confirms both retain StartBringItemIntoView in StartTailRequest. The workaround is not present in either inspected version. (src/OpenClaw.Tray.WinUI/Chat/ReactorItemsViewScrollController.cs:269, 84928c4370bc)
  • Dependency boundary: The application directly uses Microsoft.UI.Xaml controls and pins Microsoft.WindowsAppSDK 2.4.0, matching the version identified by the upstream reproducer. The WinUI anchor contract is therefore relevant to this patch. (Directory.Build.props:4, 1091062a0987)
  • Native retained-anchor mechanism: The upstream ItemsView implementation submits its retained bring-into-view element as the scroll anchor without validating that element here. The linked report provides a two-item reproducer and matching native crash stacks: ItemsView crashes with E_INVALIDARG when a pending bring-into-view target is collapsed microsoft/microsoft-ui-xaml#11865. (src/controls/dev/ItemsView/ItemsView.cpp:972, e8442d07ae57)
  • Native rejection of invalid anchors: The anchor setter throws E_INVALIDARG when the supplied element fails the scroll presenter's anchor validation, supporting the reported crash mechanism. (controls/dev/ScrollPresenter/ScrollingAnchorRequestedEventArgs.cpp:43, 6d4e2040ea8f)
  • Real setup evidence and explicit coverage limits: The complete supplied PR body reports an isolated ARM64 Debug app using two synthetic 77-message histories, one successful selection, responsiveness, and copied final-row text. It expressly states that repeated switching stopped because the selector failed and that streaming while scrolled up was not verified. This is positive native runtime evidence, but incomplete coverage of the crash trigger. Captured context sourceRevision: e8dd4a9d1312f14eba1487c684fe8b1ec156a0fbc1447b32231b8c9b9ba7ae6b. No local media files were available in the supplied scratch directory. (1091062a0987)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Complete current-head native proof of repeated mixed-height session switching, final-row visibility, and preserved scroll-away behavior during new arrivals.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant