Skip to content

feat(chat): migrate chat surface to Reactor - #1055

Open
karkarl wants to merge 15 commits into
openclaw:mainfrom
karkarl:karkarl-add-reactor-dependency
Open

feat(chat): migrate chat surface to Reactor#1055
karkarl wants to merge 15 commits into
openclaw:mainfrom
karkarl:karkarl-add-reactor-dependency

Conversation

@karkarl

@karkarl karkarl commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

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

  • ChatPage and ChatWindow mount ReactorHostControl directly into their existing XAML chat hosts.
  • OpenClawReactorChatRoot owns provider subscription, session selection, history state, composer callbacks, voice/mute state, tool-call visibility, and attachment injection.
  • The default route no longer nests Reactor under FunctionalUI.

Timeline and message presentation

  • The timeline uses keyed Reactor ItemsView + ItemContainer realization and a native AnnotatedScrollBar bound through ItemsView.VerticalScrollController.
  • Message containers are non-selectable and their container hover/selection visuals are neutralized while message-level actions remain available.
  • User and assistant bubbles follow the seeded Fluent design, with assistant-run grouping and agent identity avatar fallback.
  • Initial history tail positioning is requested after the ItemsView, its internal ScrollView, 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

  • The composer uses the seeded 8px Fluent card pattern: transparent input, subtle 32px controls, aligned picker chevrons, and a single accent primary action.
  • Session selection preserves history loading, uses stable item keys, and groups sessions in a scrollable flyout.
  • The / command palette restores leading-slash detection, catalog loading, grouping/filtering, keyboard selection, argument choices, and insertion.

Chat display settings

  • Removed the non-working Read responses aloud option from the main Settings page. Voice settings remain the supported location for that behavior.
  • Show tool calls and usage now synchronizes from every persisted settings save to all live Reactor chat hosts.
  • When disabled, tool-call rows, usage summaries, and hover time/model metadata are hidden. Copy and voice actions remain available.

Regression fixes

Review follow-up fixes

The Hanselman dual-model review findings with more than 90% fix confidence were applied:

  • Explicit send scroll tokens participate in the Reactor initial-tail request key.
  • Bitmap paste suppresses default text paste synchronously before asynchronous image extraction.
  • SoftwareBitmap and InMemoryRandomAccessStream are 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 - passed
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore - 3221 passed, 31 skipped
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore - 1969 passed
  • Focused NO_REPLY coverage: 3 shared tests and 2 tray-provider tests passed.
  • Scoped code reviews for the display-settings and NO_REPLY changes: no significant issues found.

Real behavior proof

  • Current-head isolated tray preview launches successfully through run-app-local.ps1 -NoBuild -Isolated -AllowNonMain.
  • The Suppress assistant NO_REPLY chat directives #884 regression coverage proves assistant no_reply is suppressed for both live and history paths, while user no_reply is preserved.

Not verified / blocked

  • dotnet test ./tests/OpenClaw.Tray.UITests/OpenClaw.Tray.UITests.csproj -r win-arm64 --no-restore was 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.
  • Video proof for the manual scroll scenarios is not available in this session because no computer-use/video-capture tool is exposed and Local MCP Server is disabled in the isolated tray data.

karkarl and others added 7 commits July 21, 2026 17:33
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>
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. labels Jul 25, 2026
@clawsweeper

clawsweeper Bot commented Jul 25, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed July 28, 2026, 12:09 AM ET / 04:09 UTC.

ClawSweeper review

What this changes

This 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
Reviewed head: 1adc52be78043225d4c4f23422630a5c6c60b440
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The migration has substantive implementation and tests, but unresolved security/scope and diagnostic findings plus insufficient runtime proof keep it below merge-ready quality.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The PR body reports an isolated launch and a collaborator’s signed-in session-picker test, but no inspectable redacted recording, screenshot, terminal output, or runtime log proves the current-head initial-tail, streaming follow, scroll-away, and return-to-tail behavior. Attach current-head evidence and redact private endpoints, IP addresses, and credentials; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. 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 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The PR body reports an isolated launch and a collaborator’s signed-in session-picker test, but no inspectable redacted recording, screenshot, terminal output, or runtime log proves the current-head initial-tail, streaming follow, scroll-away, and return-to-tail behavior. Attach current-head evidence and redact private endpoints, IP addresses, and credentials; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. 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 5 items Unresolved privileged workflow finding: The PR still adds a 202-line ClawSweeper dispatch workflow triggered by pull_request_target and comment activity. It mints an app token with write access to a separate repository, which is unrelated to migrating the chat renderer and remains a security-boundary and review-scope blocker.
Unresolved accessibility diagnostic finding: The tray project still suppresses four Reactor diagnostics through project-wide NoWarn settings. The explanatory comment says existing XAML surfaces are not Reactor trees, but this suppression also hides diagnostics for the newly migrated Reactor chat surface.
Architecture and policy alignment: The PR updates the architecture ledger to name ReactorChatTimeline as the production ItemsView and ItemContainer owner, which is directionally aligned with the repository’s stated ownership-transfer guardrail, but the migration still needs the required visible behavior proof.
Findings 2 actionable findings [P1] Remove the privileged dispatch workflow from this chat PR
[P2] Do not suppress Reactor diagnostics project-wide
Security Needs attention Privileged cross-repository dispatch is bundled with UI work: The added workflow uses pull_request_target and issue-comment events, then creates an app token with write access to another repository. Even without checking out PR code, this is a meaningful trusted-boundary change that needs isolated review and explicit maintainer approval.

How this fits together

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

Decision needed

Question Recommendation
Should the repository accept the new cross-repository ClawSweeper dispatch workflow as part of this native chat migration, or require it to be removed and reviewed separately? Remove and split the workflow: Remove .github/workflows/clawsweeper-dispatch.yml from this PR and submit it separately with dedicated automation and security review.

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

  • Add real behavior proof - Needs stronger real behavior proof before merge: The PR body reports an isolated launch and a collaborator’s signed-in session-picker test, but no inspectable redacted recording, screenshot, terminal output, or runtime log proves the current-head initial-tail, streaming follow, scroll-away, and return-to-tail behavior. Attach current-head evidence and redact private endpoints, IP addresses, and credentials; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. 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.
  • Remove the privileged dispatch workflow from this chat PR (P1) - This pull_request_target workflow mints a write-capable app token for the separate openclaw/clawsweeper repository from PR and comment events. It is unrelated to the Reactor migration and broadens the trusted automation boundary, so remove it and land it only through a separately reviewed automation change.
  • Do not suppress Reactor diagnostics project-wide (P2) - NoWarn disables Reactor accessibility, dialog, and theme diagnostics for the entire tray project, including the new production Reactor chat tree. Keep any compatibility handling limited to the legacy XAML surfaces or fix the reported diagnostics so the migrated surface remains checked.
  • Resolve security concern: Privileged cross-repository dispatch is bundled with UI work - The added workflow uses pull_request_target and issue-comment events, then creates an app token with write access to another repository. Even without checking out PR code, this is a meaningful trusted-boundary change that needs isolated review and explicit maintainer approval.
  • Resolve merge risk (P1) - Merging the added dispatch workflow would broaden a chat UI PR into privileged cross-repository automation that can mint a write-capable app token.
  • Resolve merge risk (P1) - The project-wide Reactor NoWarn entries can conceal accessibility and dialog/theme diagnostics in the new production chat surface.
  • Resolve merge risk (P1) - The migration replaces session selection, history tailing, virtualized scrolling, and streaming presentation across both native chat hosts; the PR supplies assertions and reported runs but no inspectable current-head artifact proving user scroll-away and return-to-tail behavior.
  • Resolve merge risk (P1) - A preview Microsoft.UI.Reactor dependency becomes part of the default native chat runtime, so upgrade and accessibility behavior need visible validation beyond source-contract tests.
  • Complete next step (P2) - A maintainer must decide whether the cross-repository automation belongs here, while the contributor must supply real native UI proof that automation cannot produce for their environment.
  • Improve patch quality - Remove the unrelated privileged dispatch workflow from this branch.
  • Improve patch quality - Replace project-wide Reactor NoWarn suppression with narrow compatibility handling or resolved diagnostics.
  • Improve patch quality - Attach redacted current-head native UI proof for initial tail, streaming follow, scroll-away, and return-to-tail.

Findings

  • [P1] Remove the privileged dispatch workflow from this chat PR — .github/workflows/clawsweeper-dispatch.yml:11
  • [P2] Do not suppress Reactor diagnostics project-wide — src/OpenClaw.Tray.WinUI/OpenClaw.Tray.WinUI.csproj:101
  • [high] Privileged cross-repository dispatch is bundled with UI work — .github/workflows/clawsweeper-dispatch.yml:11
Agent review details

Security

Needs attention: The chat migration adds unrelated privileged automation that creates a cross-repository write-token path from PR and comment events.

Review metrics

Metric Value Why it matters
Patch size 4,559 added, 604 removed, 42 files affected The default native chat path changes across both hosts, provider-facing presentation, settings, tests, and documentation.
Privileged automation 1 added workflow, 202 lines A cross-repository app-token dispatch path is materially outside the stated chat-renderer migration.
Runtime dependency 1 preview UI package added Microsoft.UI.Reactor becomes a dependency of the default native chat surface.

Merge-risk options

Maintainer options:

  1. Split privileged automation and tighten diagnostics (recommended)
    Remove the dispatch workflow from this PR and scope Reactor diagnostic handling so the new production surface retains applicable accessibility checks.
  2. Accept explicit migration risk
    A maintainer may accept the renderer and automation scope together only with explicit approval of the cross-repository token boundary and the preview-runtime upgrade path.
  3. Pause for narrower delivery
    Pause this branch if visible scroll proof and a chat-only reviewable scope cannot be supplied without retaining unrelated automation changes.

Technical review

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

  • [P1] Remove the privileged dispatch workflow from this chat PR — .github/workflows/clawsweeper-dispatch.yml:11
    This pull_request_target workflow mints a write-capable app token for the separate openclaw/clawsweeper repository from PR and comment events. It is unrelated to the Reactor migration and broadens the trusted automation boundary, so remove it and land it only through a separately reviewed automation change.
    Confidence: 0.98
  • [P2] Do not suppress Reactor diagnostics project-wide — src/OpenClaw.Tray.WinUI/OpenClaw.Tray.WinUI.csproj:101
    NoWarn disables Reactor accessibility, dialog, and theme diagnostics for the entire tray project, including the new production Reactor chat tree. Keep any compatibility handling limited to the legacy XAML surfaces or fix the reported diagnostics so the migrated surface remains checked.
    Confidence: 0.97

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against fe43b157b561.

Labels

Label justifications:

  • P2: This is a substantial user-facing chat migration with blocking review and proof work, but no demonstrated active outage.
  • merge-risk: 🚨 compatibility: The PR replaces the default native chat implementation and introduces a preview runtime dependency, so existing user behavior can regress across upgrades.
  • merge-risk: 🚨 session-state: Session selection, history hydration, initial tail positioning, and streaming follow are all reimplemented in the new renderer.
  • merge-risk: 🚨 security-boundary: The PR adds a pull_request_target and comment-triggered workflow that mints a write-capable token for a separate repository.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body reports an isolated launch and a collaborator’s signed-in session-picker test, but no inspectable redacted recording, screenshot, terminal output, or runtime log proves the current-head initial-tail, streaming follow, scroll-away, and return-to-tail behavior. Attach current-head evidence and redact private endpoints, IP addresses, and credentials; updating the PR body should trigger re-review, or a maintainer can request @clawsweeper re-review. 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

Security concerns:

  • [high] Privileged cross-repository dispatch is bundled with UI work — .github/workflows/clawsweeper-dispatch.yml:11
    The added workflow uses pull_request_target and issue-comment events, then creates an app token with write access to another repository. Even without checking out PR code, this is a meaningful trusted-boundary change that needs isolated review and explicit maintainer approval.
    Confidence: 0.98

What I checked:

  • Unresolved privileged workflow finding: The PR still adds a 202-line ClawSweeper dispatch workflow triggered by pull_request_target and comment activity. It mints an app token with write access to a separate repository, which is unrelated to migrating the chat renderer and remains a security-boundary and review-scope blocker. (.github/workflows/clawsweeper-dispatch.yml:11, 1adc52be7804)
  • Unresolved accessibility diagnostic finding: The tray project still suppresses four Reactor diagnostics through project-wide NoWarn settings. The explanatory comment says existing XAML surfaces are not Reactor trees, but this suppression also hides diagnostics for the newly migrated Reactor chat surface. (src/OpenClaw.Tray.WinUI/OpenClaw.Tray.WinUI.csproj:101, 1adc52be7804)
  • Architecture and policy alignment: The PR updates the architecture ledger to name ReactorChatTimeline as the production ItemsView and ItemContainer owner, which is directionally aligned with the repository’s stated ownership-transfer guardrail, but the migration still needs the required visible behavior proof. (docs/ARCHITECTURE.md:74, 1adc52be7804)
  • Prior review continuity: The prior completed review at 2a42ffe identified removal of the privileged workflow as P1 and scoping Reactor warning suppression as P2. Both changed files remain part of the current head diff, so the blockers remain rather than being newly raised. (.github/workflows/clawsweeper-dispatch.yml:11, 1adc52be7804)
  • Related merged behavior: The branch preserves behavior related to the merged NO_REPLY change and earlier scroll work, but its own body identifies stronger streaming-follow behavior as remaining follow-up work in a separate open issue. (src/OpenClaw.Shared/Models.cs:1803, 1adc52be7804)

Likely related people:

  • karkarl: Authored the prior merged native-chat scrolling work and the Reactor migration, giving them the strongest history signal for the chat rendering and timeline behavior. (role: feature owner; confidence: high; commits: 653327abe5ed, 1adc52be7804; files: src/OpenClaw.Tray.WinUI/Chat/ReactorChatTimeline.cs, src/OpenClaw.Tray.WinUI/Chat/OpenClawReactorChatRoot.cs)
  • TurboTheTurtle: Authored the merged shared and tray NO_REPLY suppression work that this branch extends to lowercase directives. (role: adjacent feature contributor; confidence: medium; commits: 65b88d8b639f; files: src/OpenClaw.Shared/Models.cs, tests/OpenClaw.Shared.Tests/OpenClawGatewayClientTests.cs)
  • shanselman: Tested the branch with signed-in session data and pushed the focused session-picker visibility correction now included in the current PR head. (role: recent area reviewer and contributor; confidence: high; commits: b8822f9a046a; files: src/OpenClaw.Tray.WinUI/Services/SessionVisibilityFilter.cs, tests/OpenClaw.Tray.Tests/SessionVisibilityFilterTests.cs)

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 (5 earlier review cycles)
  • reviewed 2026-07-25T00:30:38.245Z sha 2c7fc5b :: needs real behavior proof before merge. :: [P2] Avoid suppressing Reactor accessibility diagnostics project-wide
  • reviewed 2026-07-25T05:05:08.095Z sha b515013 :: needs real behavior proof before merge. :: [P2] Scope Reactor diagnostic suppression to legacy surfaces
  • reviewed 2026-07-25T05:46:33.757Z sha b515013 :: needs real behavior proof before merge. :: [P2] Scope Reactor diagnostic suppression to legacy surfaces
  • reviewed 2026-07-27T19:58:58.244Z sha b8822f9 :: needs real behavior proof before merge. :: [P2] Scope Reactor warning suppression to legacy surfaces
  • reviewed 2026-07-28T02:47:02.249Z sha 2a42ffe :: needs real behavior proof before merge. :: [P1] Remove the privileged dispatch workflow from this chat PR | [P2] Scope Reactor warning suppression to legacy surfaces

karkarl and others added 2 commits July 24, 2026 18:33
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@karkarl

karkarl commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Hanselman review follow-up

Two independent reviews found no overlapping issue, but we accepted and fixed the three high-confidence findings:

  1. Explicit send-to-latest: the Reactor timeline's initial-tail request key now includes ScrollToBottomToken, so a successful send issues a supported tail bring request.
  2. Image paste suppression: the paste handler now synchronously checks for a bitmap and sets Handled before awaiting extraction, preventing default text paste from being inserted alongside an image attachment.
  3. Native image resource cleanup: SoftwareBitmap and InMemoryRandomAccessStream are now disposed via using declarations.

Validation after the fixes:

  • ./build.ps1
  • Shared tests: 3221 passed, 31 skipped
  • Tray tests: 1961 passed
  • ARM64 UI suite initially hit two order-dependent accessibility fixture failures; both pass in isolated reruns.

The remaining lower-confidence review findings were deliberately not changed in this pass.

@karkarl
karkarl marked this pull request as ready for review July 25, 2026 05:13
steipete and others added 2 commits July 26, 2026 18:53
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 7b9bc681-ce19-4a8b-b06c-94efb63a5f9f
@shanselman

Copy link
Copy Markdown
Collaborator

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 done/ended status as its visibility rule. That hid useful completed chats while leaving idle zero-usage sessions with unknown status in the menu. It now keeps the selected session and sessions with actual activity or token usage regardless of lifecycle status, while the existing presentation policy continues to hide background sessions.

Validation on the pushed head:

  • ./build.ps1: passed
  • Shared tests: 3,221 passed, 31 skipped
  • Tray tests: 1,961 passed
  • Signed-in UI proof: completed chats appeared in the picker; idle zero-usage placeholders were absent
  • Rubber-duck review: no blocking or high-confidence issues remain

The tests also cover completed chats, working sessions, input/output/total token signals, selected empty sessions, and context-capacity-only placeholders.

karkarl and others added 3 commits July 27, 2026 14:11
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. label Jul 28, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 28, 2026
@shanselman

Copy link
Copy Markdown
Collaborator

Re-review against current head 1adc52be, merged locally with current main for validation.

The earlier session-picker, slash-command, display-settings, and NO_REPLY issues are fixed. The ClawSweeper privileged-dispatch-workflow finding is stale: .github/workflows/clawsweeper-dispatch.yml is not in the authoritative origin/main...HEAD net diff.

I still do not think this is merge-ready:

  1. Streaming tail-follow and scroll-away intent were not migrated. ReactorChatTimeline only changes its tail request key for session/generation/history replacement or an explicit-send ScrollToBottomToken. Incremental assistant streaming has no follow-mode state, and the new controller has no user scroll-away gate. The legacy production timeline preserves both invariants. Issue Add a bottom-origin virtualizing chat layout #1054 explicitly lists their restoration as acceptance criteria, so merging this PR would knowingly ship the regression rather than merely defer an enhancement. History replacement also reissues a tail bring instead of preserving prepend position.
  2. The returning-user zero-state settling guard was dropped. The legacy root waits for welcome eligibility to remain stable before showing suggestions, specifically to prevent a flash during reconnect/session materialization. The Reactor root renders empty mode immediately from entries.Count == 0, which reintroduces that race.
  3. The global Reactor suppression hides defects in the newly migrated production code. Rebuilding without the four suppressed codes reports REACTOR_A11Y_002 in ReactorChatTimeline.cs at the empty-state and assistant-avatar images (lines 334 and 521), plus REACTOR_THEME_004 at lines 709 and 728. The suppression therefore is not limited to unrelated legacy surfaces.

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:

  • ./build.ps1
  • Shared: 3,221 passed, 31 skipped
  • Tray: 1,970 passed

Recommendation: hold for the three numbered fixes plus direct runtime coverage for initial tail, streaming follow, scroll-away, and history replacement. Confidence: 95%.

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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. 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.

3 participants