Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 18, 2026, 8:42 PM ET / September 19, 2026, 00:42 UTC (Revision 2). ClawSweeper reviewWhat this changesRemoves the extra layout-event wait before Windows chat history requests scrolling to the latest message and updates the source-contract tests. Merge readiness⛔ Blocked before merge - 3 items remain Keep open: the change remains distinct from current main, and no concrete introduced defect was found. The supplied native run provides useful negative diagnostics but does not demonstrate the intended improvement. Priority: P2 Review scores
Verification
How this fits togetherThe Windows chat timeline turns history and session selections into virtualized message rows. Its scroll controller waits for native controls to load, validates the requested message, and asks WinUI to bring it into view. flowchart TD
A[History or session selection] --> B[Virtualized message rows]
B --> C[Initial scroll controller]
C --> D{Native controls loaded?}
D -->|No| E[Wait for load]
E --> C
D -->|Yes| F[Dispatch and validate request]
F --> G[Bring latest message into view]
Before merge
Agent review detailsSecurityNone. Review metricsNone. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep initial navigation in the existing controller, preserving its lifecycle guards and native row targeting, with positive evidence that removing the layout wait improves history opening. Do we have a high-confidence way to reproduce the issue? Unclear for an observable native stall: current main demonstrably waits for another layout event, but the supplied run does not isolate that wait as the cause of failed navigation. Is this the best way to solve the issue? Yes at the source-design level: this removes a prerequisite within the existing owner without adding a competing scrolling implementation. Native success on the committed dependencies remains unproven. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against a7d6c3f8d4d6. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Problem This Solves
Opening or switching chat histories can leave the initial scroll request waiting for another layout event even though the chat view is already loaded.
User Impact
User impact: initial scrolling toward the latest message no longer requires an additional layout event after the view loads. This change does not guarantee successful tail positioning or fix the existing native ItemsView invalid-anchor crash.
Why This Change Was Made
The native bring-into-view request handles target realization and layout. Waiting for an unrelated future
LayoutUpdatednotification adds an unnecessary prerequisite.InitialTailPositionerremains the owner. It waits for the ItemsView and ScrollView to load, then retains the existing dispatcher handoff so navigation runs outside Reactor reconciliation. Request-generation and tail-identity checks, unload/disposal cleanup, and streaming follow behavior remain in place. The source-contract tests now guard the load-only flow and prohibit reintroducing the layout-event subscription.No Reactor package upgrade, local Reactor patch,
ScrollToworkaround, or debugger-startup change is included.Evidence
Actual OpenClaw was exercised with two synthetic 77-message histories. That experiment reproduced the original invalid-anchor crash on the first session switch, rather than proving this change fixes it. The full native dump resolved to:
Pre-publication rubber-duck review of the committed two-file change found no actionable issues. The bundled Codex autoreview command was also attempted but could not run because the Codex CLI is not installed; that tool is not being reported as passed.
Change Type
Scope
winnodeRequired proof pools
windows-winui-interactive: initial chat-history scrolling, session switching, and native ItemsView behavior require an interactive Windows app.windows-11-arm64: native ARM64 build and runtime crash investigation were performed.Validation
.\build.ps1 -Configuration Releasedotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csprojdotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csprojThe first test runs restored packages because this was a fresh worktree. Repository-root discovery and tray settings were isolated. The tests include
ReactorTimeline_UsesStableBottomAnchoringAndDiscreteTailRequestsandReactorTimeline_InitialTailWaitsForLoadWithoutLayoutSubscription; these are source-contract checks, not proof that native scrolling always completes.Real Behavior Proof
f683edcae3b4405316149980b78a28ea5f49291f, with the explicitly noted temporary package/Markdown compatibility overlay. The committed scroll controller was unchanged.completedSwitches: 0,processAlive: false, and two history responses.N/A. The relevant native diagnostic output is copied above; the local screenshot and full dump are not publicly uploaded.Security Impact
NoNoNoNoNoYes, explain the risk and mitigation: N/A.Compatibility and Migration
YesNoNoReview Conversations