Skip to content

fix(chat): stabilize bottom anchoring - #1089

Merged
bkudiess merged 4 commits into
openclaw:mainfrom
karkarl:karkarl-investigate-chat-layout-crash
Aug 5, 2026
Merged

fix(chat): stabilize bottom anchoring#1089
bkudiess merged 4 commits into
openclaw:mainfrom
karkarl:karkarl-investigate-chat-layout-crash

Conversation

@karkarl

@karkarl karkarl commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR stabilizes the Reactor chat follow-tail path and hardens queued native navigation against stale reconciled state.

  • Establishes native bottom anchoring once when the inner ScrollView attaches.
  • Keeps ViewChanged observation-only.
  • Stops issuing native navigation as streamed text grows in place.
  • Uses stable row keys to distinguish streamed growth from a genuinely new tail row.
  • Captures immutable tail requests and revalidates index, identity, item count, and generation immediately before StartBringItemIntoView.
  • Clears invalid pending work on empty-tail transitions, unload, and disposal while preserving empty-to-valid follow behavior.
  • Synchronizes the branch with current main, including refactor(chat): use Reactor theme resources #1084 theme-resource changes, without conflicts.

Tail navigation contract

TailNavigationPolicy.TryCapture rejects empty or out-of-range tail state before work is queued. TailNavigationQueue coalesces to the newest generation while retaining one dispatcher callback. TailNavigationPolicy.CanExecute then rejects stale identity, count, generation, or bounds immediately before native navigation.

The design intentionally has no synthetic sentinel row, custom layout, extent calculation, TryGetItemIndex, ChangeView, UpdateLayout, streamed text-length trigger, settle loop, or timer. ItemsView owns virtualization, ScrollView owns scrolling and anchoring, and ViewChanged remains observation-only.

Validation

Current synchronized head: 12bc6d2c

  • git diff --check
  • .\build.ps1: passed
  • Shared: 3,399 passed, 32 skipped
  • Tray: 2,045 passed
  • Focused chat timeline and tail-policy tests: 15 passed
  • WinUI non-accessibility: 100 passed
  • WinUI accessibility: 19 passed
  • The synchronized diff against current main remains limited to the intended five files.
  • Fresh GitHub CI passed: repository hygiene, shared/tray and UI tests, setup-connect E2E, revocation-recovery E2E, network-recovery E2E, win-x64 build, win-arm64 build, dispatch, and the CodeQL advanced-setup gate.

Real behavior proof

The maintainer accepts the contributor's manual proof for be07589d, the functional commit immediately below the synchronization merge:

  • Clean debugger-free ARM64 DevBuild with isolated tray data.
  • 25 full-range scroll/remount passes.
  • 15 additional remount passes with 2,846 tool-card expand/collapse operations.
  • A 14,077-character assistant response streamed to completion while 12 alternating Home/End passes and a Chat remount ran.
  • The app remained responsive.
  • Signed DebugView and application logs contained no LayoutCycleException, Element is already the child of another element, COM failure, or unhandled exception.

The synchronization commit only incorporates current main and merged cleanly. Exact synchronized-head build, Tray, Shared, focused timeline, WinUI, accessibility, x64, ARM64, and E2E validation all pass.

The earlier reviewer-observed exception on 96f0e9ec remains classified as an intermittent negative-reproduction concern rather than a current known code defect. It did not reproduce in clean isolation passes or the contributor's current-head stress runs.

Review

  • The confirmed stale queued-tail race was fixed in be07589d.
  • ClawSweeper found no remaining actionable code findings on that functional head.
  • Final synchronized-diff GPT review found no actionable findings and returned MERGE at 88% confidence.
  • Final synchronized-diff Gemini review found no significant issues and returned MERGE at 100% confidence.
  • Maintainer accepts the manual proof. Combined merge confidence is above the repository's 90% threshold.

Scope

This PR changes only Reactor chat tail positioning and its focused policy/tests. Theme and navigation resources are supplied by current main through #1084.

karkarl and others added 2 commits August 4, 2026 07:37
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: 🦪 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. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Aug 4, 2026
@clawsweeper

clawsweeper Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 4, 2026, 8:08 PM ET / August 5, 2026, 00:08 UTC.

ClawSweeper review

What this changes

The PR replaces dynamic chat follow-tail anchoring with stable native bottom anchoring and validates queued navigation against the current tail row and item count.

Regression provenance

Possible regression — probable (reviewed change; failure trace). No predecessor PR is attributed.

Merge readiness

Blocked until stronger real behavior proof is added - 4 items remain

Keep open pending inspectable current-head runtime proof. The queue-boundary repair is focused, but the linked proof artifact could not be retrieved and an earlier exact-head review recorded an intermittent layout exception.

Priority: P1
Reviewed head: be07589d3df80bd07e1464fb18e5585b73aa646d

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The source repair is focused, but independently inspectable real-behavior proof remains a merge gate.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The PR describes a current-head runtime run, but the linked artifact was unreachable here and prose alone cannot directly establish the changed scroll behavior. 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 PR describes a current-head runtime run, but the linked artifact was unreachable here and prose alone cannot directly establish the changed scroll behavior. 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 Current-head request validation: The PR head captures a tail index and stable row key, then rechecks bounds, identity, and current item count immediately before invoking native navigation.
Focused policy coverage: The PR adds unit coverage for valid-to-empty, empty-to-valid, stale identity, out-of-range bounds, generation replacement, and stale-generation rejection.
Feature provenance: The current Reactor chat timeline dates to the merged Reactor migration, which is the relevant current-main ownership path.
Findings None None.
Security None None.

How this fits together

The Windows tray chat renders keyed conversation rows in a virtualized native list. Timeline updates either preserve reading position during in-place streaming or request native navigation for an initial load or genuinely new final row.

flowchart LR
A[Chat timeline updates] --> B[Keyed message rows]
B --> C[Virtualized native list]
C --> D{Initial load or new tail?}
D -->|yes| E[Validate queued request]
D -->|no| F[Observe reading position]
E --> G[Native tail navigation]
F --> H[Native bottom anchoring]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: The PR describes a current-head runtime run, but the linked artifact was unreachable here and prose alone cannot directly establish the changed scroll behavior. 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) - The intermittent layout exception observed on the prior stable-anchor head has not been independently ruled out on the current head.
  • Resolve merge risk (P1) - Persistent native anchoring changes scroll-away and remount behavior, so it needs direct current-head evidence beyond policy-unit tests.
  • Complete next step (P2) - This needs contributor-supplied, inspectable current-head runtime proof rather than an automated code repair.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch size 5 files, +324/-50 lines The PR replaces central chat scrolling behavior and its focused test coverage.
Production versus tests production +224/-47, tests +100/-3 Most added code is the scrolling policy, with a substantial focused regression-test addition.

Merge-risk options

Maintainer options:

  1. Attach inspectable runtime evidence (recommended)
    Provide a redacted current-head recording or diagnostic capture that directly demonstrates scrolling away, streaming growth, new-tail follow, and long-history remounting without the reported exception.

Technical review

Best possible solution:

Retain the bounded tail-request design and add redacted, inspectable current-head proof covering scroll-away, in-place streaming, new-tail follow, and long-history remounting before merge.

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

Unclear. A collaborator observed an exact-head layout exception, while the contributor's subsequent long run did not reproduce it and its artifact was unavailable for inspection.

Is this the best way to solve the issue?

Unclear. The source repair correctly narrows stale queued navigation, but the stable-anchor runtime behavior still needs direct current-head evidence.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P1: A reported unhandled layout exception can interrupt the active chat workflow.
  • merge-risk: 🚨 compatibility: The PR changes established follow-tail and scroll-away behavior for existing chat users.
  • merge-risk: 🚨 availability: The modified native scrolling path is associated with a reported intermittent WinUI exception.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish 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 PR describes a current-head runtime run, but the linked artifact was unreachable here and prose alone cannot directly establish the changed scroll behavior. 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:

Likely related people:

  • karkarl: Authored the merged Reactor chat migration and the active follow-tail work. (role: introduced current chat surface; confidence: high; commits: 9acafa606a4d, be07589d3df8; files: src/OpenClaw.Tray.WinUI/Chat/ReactorChatTimeline.cs, src/OpenClaw.Tray.WinUI/Chat/ReactorItemsViewScrollController.cs)
  • bkudiess: Provided the exact-head review that identified the layout-exception and stale queued-tail paths. (role: recent reviewer; confidence: high; files: src/OpenClaw.Tray.WinUI/Chat/ReactorItemsViewScrollController.cs)

Rank-up moves

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

  • Attach redacted current-head runtime proof for scroll-away, streaming growth, new-tail follow, and long-history remounting.
  • Include visible diagnostics or a log excerpt showing no layout exception during that run.

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-08-04T17:12:05.188Z sha 96f0e9e :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-04T17:17:17.991Z sha 96f0e9e :: needs real behavior proof before merge. :: [P1] Release the bottom anchor after scroll-away
  • reviewed 2026-08-04T18:19:49.116Z sha 96f0e9e :: needs real behavior proof before merge. :: [P1] Release bottom anchoring after scroll-away
  • reviewed 2026-08-04T19:02:42.213Z sha 96f0e9e :: needs real behavior proof before merge. :: [P1] Release bottom anchoring after scroll-away
  • reviewed 2026-08-04T22:04:28.120Z sha 96f0e9e :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Aug 4, 2026
@karkarl
karkarl marked this pull request as ready for review August 4, 2026 18:24
@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 Aug 4, 2026
@bkudiess

bkudiess commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

I completed a dual-model adversarial review plus exact-head paired-profile testing of 96f0e9ec.

The anchoring direction is promising, but I do not think this is merge-ready at our >90% confidence bar.

What passed

  • Full build passed.
  • Shared: 3,399 passed, 32 skipped.
  • Tray: 2,037 passed.
  • Scroll-away during streaming passed: the same assistant entry grew from 1,141 to 2,693 characters while the captured viewport remained pixel-identical.
  • New-tail follow passed: a new assistant row remained followed while growing from 679 to 2,305 characters.
  • A real session_status tool card survived eight expand/collapse cycles.

Blocking before merge

Exact-head layout exception

During prolonged long-conversation remount/virtualization scrolling, the exact PR head logged an unhandled:

Microsoft.UI.Xaml.LayoutCycleException
Element is already the child of another element.

The app remained responsive, and clean follow-up runs did not reproduce it during idle Chat, ordinary scrolling, or a shorter UI Automation pass. The precise trigger therefore remains unresolved, but a current-head unhandled layout exception prevents approval.

Queued tail index can become invalid

UpdateTail changes _tailIndex without synchronizing _valid. A queued callback can consequently observe stale validity and call StartBringItemIntoView(-1) after the displayed rows become empty. Tool-only timelines are supported, and hiding tool rows provides a reachable zero-row state. WinUI throws for an out-of-range index.

Please capture the queued index, revalidate it against the current item count when the callback executes, and keep _valid synchronized during tail transitions.

Requested follow-up

  1. Fix the stale tail-validity/index path and add behavioral coverage for valid-to-empty and empty-to-valid transitions.
  2. Add a Reactor runtime regression covering long-history remounting, scrolling, streaming, and tool expansion.
  3. Reproduce or eliminate the observed LayoutCycleException.
  4. Attach inspectable current-head logs and interaction proof.

Current merge confidence: 45%. Requesting changes.

Capture immutable tail requests, revalidate their index and row identity against the reconciled item count, and coalesce dispatcher work without allowing stale generations to execute.

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

karkarl commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the stale queued-tail race in be07589d.

The positioner now receives the reconciled rows.Count, captures an immutable (index, displayedTailKey) request only when the index is in range, coalesces work through a generation-aware TailNavigationQueue, and revalidates index, identity, count, and generation immediately before StartBringItemIntoView. Empty-tail transitions, unload, and disposal clear pending work without preventing a newer valid request from being consumed by the already-scheduled callback.

Added behavioral coverage for valid-to-empty, empty-to-valid, stale identity, out-of-range bounds, callback replacement, and stale generations. Validation: full build passed; Shared 3,399 passed / 32 skipped; Tray 2,042 passed; focused tail policy tests 5 passed.

Current-head runtime attempt used a clean debugger-free ARM64 DevBuild and isolated profile. It completed 25 full-range scroll/remount passes plus 15 remount passes with 2,846 tool-card expand/collapse operations. The app remained responsive. The signed DebugView capture and app log contained no LayoutCycleException, Element is already the child of another element, or unhandled exception. The intermittent exact-head exception did not reproduce in this run, so I have recorded that as negative reproduction evidence rather than claiming the separate trigger is eliminated.

@karkarl

karkarl commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Additional streaming proof on the same current-head instance:

  • Switched to an alternate isolated gateway session and streamed a 14,077-character assistant response to final=True.
  • During active streaming, ran 12 alternating full-range Home/End passes and remounted Chat through Sessions navigation.
  • The app remained responsive through completion.
  • The 1,288-line signed DebugView capture contains no LayoutCycleException, Element is already the child of another element, unhandled exception, or reply session initialization conflicted event.

Capture: pr-1089-tail-fix-stream-repro-dbgview.log in the session artifacts. This supplements the long-history and 2,846 tool-card churn evidence above.

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

@bkudiess bkudiess left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed and live-tested with >90% confidence after the queued-tail repair and synchronization with current main. The stale index/generation path is now bounded by immutable request capture and immediate execution-time validation. Exact synchronized-head build, 3,399 Shared tests (32 skipped), 2,045 Tray tests, 15 focused chat/tail tests, 100 non-accessibility UI tests, and 19 accessibility tests passed. Fresh GitHub CI is green for x64, ARM64, tests, all E2E shards, hygiene, dispatch, and the CodeQL gate. Maintainer accepts the extensive current-head manual remount, scrolling, streaming, and tool-card stress proof. Final GPT and Gemini reviews found no actionable issues.

@bkudiess
bkudiess merged commit ca8fb4a into openclaw:main Aug 5, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P1 Urgent regression or broken agent/channel workflow affecting real users now. 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.

2 participants