feat(chat): migrate chat surface to Reactor - #1055
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs real behavior proof before merge. Reviewed July 28, 2026, 12:09 AM ET / 04:09 UTC. ClawSweeper reviewWhat this changesThis PR replaces the default native WinUI chat renderer with Microsoft UI Reactor, updating the timeline, composer, session picker, chat settings synchronization, silent-directive handling, and related tests. Merge readiness⛔ Blocked until stronger real behavior proof is added - 13 items remain Keep this PR open for a focused repair and real-behavior proof. The latest head still carries the two previously identified blockers: an unrelated privileged cross-repository dispatch workflow and project-wide suppression of Reactor accessibility diagnostics; the large native chat replacement also lacks inspectable current-head evidence for its scroll and streaming behaviors. Priority: P2 Review scores
Verification
How this fits togetherThe tray’s native chat page and pop-out window subscribe to gateway-backed chat snapshots, render sessions and message history, and send user actions back through the shared provider. This PR swaps the presentation layer from FunctionalUI to Reactor while retaining the gateway/provider path and existing WinUI shell. flowchart LR
Gateway[Gateway chat events] --> Provider[Shared chat data provider]
Provider --> Snapshot[Chat snapshots and history]
Snapshot --> Reactor[Reactor chat surface]
Reactor --> Timeline[Virtualized message timeline]
Reactor --> Composer[Composer and commands]
Composer --> Provider
Reactor --> WinUI[Chat page and pop-out window]
Decision needed
Why: The workflow changes the repository’s trusted automation boundary and creates a write-capable token path to another repository; that is a maintainer-owned security and operational decision, not an incidental UI implementation choice. Before merge
Findings
Agent review detailsSecurityNeeds attention: The chat migration adds unrelated privileged automation that creates a cross-repository write-token path from PR and comment events. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land a chat-only Reactor migration after removing the unrelated privileged workflow, limiting any diagnostic suppression to genuinely legacy code, and attaching redacted current-head proof that covers initial history position, streaming follow, user scroll-away, and return-to-tail in both native hosts. Do we have a high-confidence way to reproduce the issue? No high-confidence complete reproduction is attached. Source and tests show the changed chat paths, but the important scroll and streaming behaviors require a current-head native UI run with signed-in or representative history data. Is this the best way to solve the issue? No. A direct Reactor renderer can be a viable direction, but this PR should not bundle privileged automation or blanket diagnostic suppression, and its runtime scroll behavior needs visible proof before the replacement is accepted. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fe43b157b561. LabelsLabel justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Hanselman review follow-upTwo independent reviews found no overlapping issue, but we accepted and fixed the three high-confidence findings:
Validation after the fixes:
The remaining lower-confidence review findings were deliberately not changed in this pass. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7b9bc681-ce19-4a8b-b06c-94efb63a5f9f
|
Hi @karkarl, I pushed a focused session-picker fix onto this branch after testing the PR against real signed-in session data. The picker was using Validation on the pushed head:
The tests also cover completed chats, working sessions, input/output/total token signals, selected empty sessions, and context-capacity-only placeholders. |
…add-reactor-dependency
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Re-review against current head The earlier session-picker, slash-command, display-settings, and I still do not think this is merge-ready:
Two smaller parity gaps are also worth fixing while this is open: queued messages lost their bounded scroll region and polite live-region summary, and a command-catalog refresh can reopen a slash palette that the user dismissed while the input still begins with Current-main merged validation passed:
Recommendation: hold for the three numbered fixes plus direct runtime coverage for initial tail, streaming follow, scroll-away, and history replacement. Confidence: 95%. |
Summary
This PR migrates the default native chat experience from FunctionalUI to a direct Microsoft.UI.Reactor surface while preserving the existing gateway/provider model and surrounding WinUI chrome.
Direct Reactor chat surface
ChatPageandChatWindowmountReactorHostControldirectly into their existing XAML chat hosts.OpenClawReactorChatRootowns provider subscription, session selection, history state, composer callbacks, voice/mute state, tool-call visibility, and attachment injection.Timeline and message presentation
ItemsView+ItemContainerrealization and a nativeAnnotatedScrollBarbound throughItemsView.VerticalScrollController.ItemsView, its internalScrollView, and first usable layout are ready. This preserves the behavior addressed by Fix chat scrollbar not reaching bottom during streaming via scroll anchoring #1014 without reintroducing height calculations,ChangeView,UpdateLayout,VerticalAnchorRatio, or timer-based settling.Composer, sessions, and commands
/command palette restores leading-slash detection, catalog loading, grouping/filtering, keyboard selection, argument choices, and insertion.Chat display settings
Regression fixes
NO_REPLYand lowercaseno_replyare suppressed from gateway live events and history hydration before the Reactor provider renders them. Literal user messages remain visible.ItemsViewlifecycle.Review follow-up fixes
The Hanselman dual-model review findings with more than 90% fix confidence were applied:
SoftwareBitmapandInMemoryRandomAccessStreamare disposed after image attachment encoding.Follow-up
A custom bottom-origin virtualizing layout and hardened streaming-follow behavior are tracked in #1054. The follow-up must preserve the annotated scrollbar controller binding while avoiding height/extent calculations,
ChangeView,UpdateLayout,VerticalAnchorRatio, and timer-driven scrolling.Validation
./build.ps1- passeddotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore- 3221 passed, 31 skippeddotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore- 1969 passedNO_REPLYcoverage: 3 shared tests and 2 tray-provider tests passed.NO_REPLYchanges: no significant issues found.Real behavior proof
run-app-local.ps1 -NoBuild -Isolated -AllowNonMain.no_replyis suppressed for both live and history paths, while userno_replyis preserved.Not verified / blocked
dotnet test ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -r win-arm64 --no-restorewas blocked after test discovery: the ARM64 UI runner hung with no individual test output or result. A prior attempt was also blocked by a stale companion-process DLL lock.