Skip to content

fix(chat): avoid session-switch crashes without clipping Markdown lists - #1426

Merged
shanselman merged 1 commit into
mainfrom
karkarl-reactor-rollback-workaround
Sep 15, 2026
Merged

shanselman merged 1 commit into
mainfrom
karkarl-reactor-rollback-workaround

Conversation

@karkarl

@karkarl karkarl commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

What Problem This Solves

Mitigates release-blocking WinUI chat session-switch crashes while keeping Markdown list content readable. Related: #1421 and #1419. Follow-up/removal tracking: #1424 (keep open).

Draft, not release-ready: full 240-message session-switch/scrolling proof and independent review remain blocked below.

User Impact

Retains existing row-targeted scrolling and virtualization. Long list items wrap instead of being clipped. No settings, credential, protocol or migration changes.

Why This Change Was Made

The reported regression begins with Reactor preview.13's ElementFactory eviction/lifetime change, microsoft/microsoft-ui-reactor@8ee940c. It exposes microsoft/microsoft-ui-xaml#11865: ItemsView retains a pending bring-into-view target after it is collapsed and subsequently throws E_INVALIDARG. Waiting for a WinUI NuGet release is not viable for the current release mitigation.

Pin both Microsoft.UI.Reactor and Microsoft.UI.Reactor.Advanced to 0.1.0-preview.12. This reverses the dependency upgrades from #1376 (build(deps): upgrade Microsoft.UI.Reactor to 0.1.0-preview.14) and #1400 (fix(chat): prevent bubble text truncation), not unrelated changes from those PRs. Adapt the Markdown factory namespace and two hit-testing setters to preview.12's APIs, preserving behavior.

Why not the scrolling workaround?

#1407 (fix(chat): work around WinUI session-switch crash) stopped the crash in reported tests, but Barbara's 240 mixed-height message scenario stopped around message 233. The bottom extent changes as rows render, so scrolling to the current extent can stop short of the actual final message. That is also a release blocker. This PR leaves ReactorItemsViewScrollController and StartBringItemIntoView unchanged.

Markdown workaround and retirement

Preview.15 supplied microsoft/microsoft-ui-reactor#1197 (Fix Markdown list content wrapping). Instead of adopting the newer row-lifetime behavior, use MarkdownOptions.ListItem to replace the default two-child HStack with an Auto marker / Star content Grid, matching that upstream layout fix. Preserve the parser-produced marker, content, nesting, formatting and existing spacing. No fixed message widths, flattening, native layout handlers or styling changes.

Old layout owner: Reactor preview.12's default list-row HStack. Temporary owner: ReactorChatTimeline.BuildSafeMarkdown's ListItem callback. Preserved invariant: only the width constraint changes. The callback explicitly rejects an unexpected dependency shape. #1424 tracks upgrading both packages and removing this callback together after a shipped WinUI fix and full runtime proof.

Evidence

Mounted native WinUI proof covers long ordered/unordered items at 240 DIPs, reflow at 600 DIPs, nested/loose lists, retained bold formatting and block content. Existing table/literal-fence and disposal tests pass.

Current-change narrow unordered-list capture, inspected locally. The full final words are visible:

Reactor preview.12 list item wrapping at 240 DIPs

Change Type

  • Bug fix
  • Tests or validation
  • Docs or instructions

Scope

  • Tray or WinUI UX
  • Tests, CI, or docs

Required proof pools

  • windows-winui-interactive: session switching, actual final-message visibility, wrapping, streaming and scroll-away behavior.
  • windows-11-arm64: dependency rollback affects native ARM64 runtime behavior; local build and mounted Markdown proof completed, full app proof outstanding.

Validation

Exact code committed as 3cda150580b1ade5448d75400ba5c252925bb824, Windows ARM64 / .NET SDK 10.0.400. OPENCLAW_REPO_ROOT points to the isolated worktree; OPENCLAW_TRAY_DATA_DIR points to a dedicated temporary test directory. Built fresh Shared/Tray test projects before --no-restore.

  • .\build.ps1: all five projects and documentation validation passed.
  • dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore --verbosity quiet: 3,983 passed, 33 skipped, 0 failed.
  • dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --verbosity quiet: 2,976 passed, 0 skipped, 0 failed.
  • Focused command below: 5 passed, 0 skipped, 0 failed.
  • git diff --cached --check: passed before commit.
dotnet test .\tests\OpenClaw.Tray.UITests\OpenClaw.Tray.UITests.csproj -r win-arm64 -p:Platform=ARM64 --filter 'FullyQualifiedName~ReactorMarkdownListProofTests|FullyQualifiedName~ReactorMarkdownTableProofTests|FullyQualifiedName~MountedReactorChatDisposalProofTests' --logger 'console;verbosity=normal'

Initial rollback compilation errors were fixed at the three incompatible API call sites, followed by full validation. Explicit test-side layout completion fixes an initial not-yet-arranged observation; production code does not call UpdateLayout.

Real behavior proof

  • Environment: native Windows ARM64, real WinUI runtime in isolated UI test fixture.
  • Head tested: 3cda150580b1ade5448d75400ba5c252925bb824 (identical code validated before commit).
  • Steps: mount production BuildSafeMarkdown; assert ordered/unordered marker and full text, finite available width and multiline height at 240 DIPs; resize to 600 DIPs and assert reduced height with identical text. Mount nested/loose content and assert three Auto/Star rows, paragraphs, quote content and bold runs. Run table and disposal proof.
  • Result: five mounted tests passed. Narrow screenshot visibly ends with the final words of the message. Other requested captures were not produced reliably; only the verified local narrow capture is included.
  • Not verified / blocked: an additional 240-message production-timeline experiment threw COMException during initial MeasureOverride/UpdateLayout in the unpackaged VSTest host, before switching. Its cause has not been established. The unsuccessful experiment is retained outside the PR, not shipped as a failing/skipped test, and not counted as crash-fix proof. Repeated live-app session switching, actual message-240 visibility, real Gateway history loading, streaming follow and reader-position preservation remain required before release. Markdown tests are not substitutes.
  • Rubber-duck / structured review blocked: python .\.agents\skills\autoreview\scripts\autoreview --mode local was attempted with rollback/proof context. After staging to fit the complete bundle, default Codex review failed with HTTP 401 Unauthorized, confirmed with a same-engine streamed retry. No clean independent review is claimed.

Security Impact

No new permissions, capabilities, network calls, command execution, data access or credential handling. Markdown sanitization, NoHtml parsing and inert image/link handling remain unchanged.

Compatibility and Migration

Both packages move together to preview.12. Existing styling is preserved. No configuration or migration is required. Keep the pin and callback until #1424's retirement gates pass.

Pin both Reactor packages to preview.12 and restore finite-width list layout through MarkdownOptions.ListItem. Preserve row-targeted scrolling and document removal gates in #1424.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 0b09606a-3b94-4974-87d1-2068b0e79e76
@clawsweeper

clawsweeper Bot commented Sep 15, 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.

@karkarl karkarl added the status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. label Sep 15, 2026
@clawsweeper clawsweeper Bot added P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 15, 2026
@clawsweeper

clawsweeper Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codex review: blocked before merge. Reviewed September 14, 2026, 10:29 PM ET / September 15, 2026, 02:29 UTC (Revision 2).

ClawSweeper review

What this changes

Pins the native chat rendering packages to preview.12 and adds a wrapping list layout, native layout tests, and workaround retirement guidance.

Merge readiness

Blocked before merge - 3 items remain

This remains a useful mitigation absent from main. The additional app screenshot strengthens Markdown proof, but does not resolve the previous review’s long-history compatibility concern. No discrete introduced code defect was found.

Priority: P0
Reviewed head: 3cda150580b1ade5448d75400ba5c252925bb824
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, source-supported mitigation with useful visual evidence, but incomplete verification of its central crash and navigation behavior.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: The external-contributor gate does not apply to this collaborator-authored PR. Inspected native screenshots positively demonstrate BuildSafeMarkdown wrapping; they do not establish session-switch recovery or long-history compatibility, which remains a separate merge decision.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The external-contributor gate does not apply to this collaborator-authored PR. Inspected native screenshots positively demonstrate BuildSafeMarkdown wrapping; they do not establish session-switch recovery or long-history compatibility, which remains a separate merge decision.
Evidence reviewed 10 items Policy and introduced scope: Read the full root AGENTS.md and required proof-validation skill. No applicable nested AGENTS.md or maintainer-notes directory was found. The verified base-to-head diff changes five files; final git status was clean. Builds and tests were not executed during this read-only review.
Actual package rollback and dependency boundary: The introduced package references move both Reactor packages from preview.15 to preview.12. BuildSafeMarkdown consumes Reactor’s ListItem callback and preserves sanitization, NoHtml, and inert image/link handling. These direct package and callback dependencies establish the relevant Microsoft Reactor contract.
Pinned dependency validates callback assumptions: The preview.12 tag resolves to this commit. LeaveListItem constructs a horizontal stack containing marker and content, then invokes ListItem; the proposed two-child shape check matches that implementation.
Findings None None.
Security None None.

How this fits together

The native Windows chat timeline turns Gateway history and streaming messages into virtualized WinUI rows. Reactor controls row reuse and Markdown layout, while the application’s scroll controller targets the final message.

flowchart LR
  A[Gateway history and streaming] --> B[Chat timeline]
  B --> C[Reactor row reuse]
  B --> D[Sanitized Markdown]
  D --> E[Wrapping list layout]
  C --> F[Native chat view]
  E --> F
  G[Final message targeting] --> F
Loading

Decision needed

Question Recommendation
Should the temporary Reactor rollback land before the declared native session-switch and long-history compatibility checks succeed? Require native compatibility results: Hold landing until current-head native results establish repeated switching, actual message-240 visibility, streaming follow, and preserved scroll-away position.

Why: Source supports the mitigation and screenshots demonstrate wrapping, but accepting an unverified renderer rollback for the crash-affected release requires explicit risk ownership.

Before merge

  • Resolve merge risk (P1) - Rolling back the renderer changes recycling for existing chat histories; successful repeated switching, final-message navigation, streaming follow, and reader-position preservation remain unverified after the reported initial-layout experiment failed.
  • Complete next step (P2) - Provide the declared current-head native session-switch and long-history compatibility results, or obtain explicit maintainer acceptance of landing with that uncertainty.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta production +20/-7; tests +148/-0; docs +25/-0 Production growth is narrowly justified by package compatibility and the temporary list-layout replacement.

Merge-risk options

Maintainer options:

  1. Verify existing-history behavior (recommended)
    Complete the already-declared native compatibility checks against the rollback before landing.
  2. Own the temporary rollback risk
    A maintainer can explicitly accept the incomplete runtime coverage for this mitigation while retaining the release gates.

Technical review

Best possible solution:

Retain the narrow layout override and row-targeted scrolling only with native upgrade-compatibility evidence, then retire both package pins and the callback together through the existing tracking issue.

Do we have a high-confidence way to reproduce the issue?

No current-main runtime reproduction was established in this review. Released-build crash reports and the upstream native reproducer support the failure mechanism, but the exact application scenario still needs successful after-fix verification.

Is this the best way to solve the issue?

Yes, provisionally: the pinned callback matches the upstream layout fix and preserves final-row targeting, avoiding the reported short-scroll alternative; the rollback’s runtime safety remains conditional on native validation.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against ddb54a9614a5.

Labels

Label justifications:

  • P0: The targeted v2026.9.3 failures terminate the Companion when users open chat history, preventing its core chat workflow.
  • merge-risk: 🚨 compatibility: Both renderer packages move backward across row-lifetime changes without completed native long-history compatibility evidence.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The external-contributor gate does not apply to this collaborator-authored PR. Inspected native screenshots positively demonstrate BuildSafeMarkdown wrapping; they do not establish session-switch recovery or long-history compatibility, which remains a separate merge decision.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The external-contributor gate does not apply to this collaborator-authored PR. Inspected native screenshots positively demonstrate BuildSafeMarkdown wrapping; they do not establish session-switch recovery or long-history compatibility, which remains a separate merge decision.

Evidence

What I checked:

Likely related people:

  • karkarl: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • azchohfi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

  • Resolve or isolate the initial-layout experiment failure and record current-head native results for the already-declared switching, message-240, streaming, and scroll-away scenarios.

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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-15T02:11:33.594Z sha 3cda150 :: blocked before merge. :: none

@karkarl

karkarl commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator Author

Screenshot as proof for markdown clipping workaround:
image

@karkarl
karkarl marked this pull request as ready for review September 15, 2026 02:26
@karkarl

karkarl commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator Author
Recording.2026-09-14.193250.mp4

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

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P0 Emergency: data loss, security bypass, crash loop, or unusable core runtime. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants