Skip to content

Virtualize chat timeline rows#898

Open
TurboTheTurtle wants to merge 6 commits into
openclaw:mainfrom
TurboTheTurtle:virtualize-chat-timeline
Open

Virtualize chat timeline rows#898
TurboTheTurtle wants to merge 6 commits into
openclaw:mainfrom
TurboTheTurtle:virtualize-chat-timeline

Conversation

@TurboTheTurtle

@TurboTheTurtle TurboTheTurtle commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Closes #860.

Adds a FunctionalUI virtual stack backed by WinUI ItemsRepeater and switches the native chat timeline rows to it, preserving existing scroll/follow handling while avoiding a StackPanel with every conversation row realized up front.

Windows CI proof on run 28481294754 mounted the real native chat timeline with 241 rows and reported CHAT_TIMELINE_VIRTUALIZATION_PROOF rows=241 itemsRepeater=ItemsRepeater layout=Vertical scrollableHeight=19967.0 verticalOffset=19967.0 newestRowVisible=true. The focused source contract test also prevents regressing the production row host back to VStack(2, timelineRows).

@clawsweeper

clawsweeper Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 30, 2026, 7:11 PM ET / 23:11 UTC.

Summary
The PR adds a FunctionalUI VirtualStackElement/VirtualVStack backed by WinUI ItemsRepeater, switches OpenClawChatTimeline rows to it, and adds contract/UI proof tests plus UI-test resource fallbacks.

Reproducibility: not applicable. this is a feature/performance PR rather than a reproducible bug report. Source inspection confirms current main and v0.6.12 still render chat rows through VStack, while the linked backlog item defines the desired virtualization behavior without a failing perf threshold.

Review metrics: 3 noteworthy metrics.

  • Changed surface: 5 files, +316/-11. The PR touches production chat rendering, the shared FunctionalUI renderer, tray contract tests, and WinUI UI test infrastructure.
  • Renderer primitive: 1 new VirtualStackElement. A shared FunctionalUI primitive can affect future call sites beyond the current chat timeline.
  • UI proof path: 1 new 241-row WinUI UI proof test. The test directly exercises the native chat timeline host and append/follow behavior added by this PR.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #860
Summary: This PR is the candidate implementation for the focused chat virtualization issue; the broader chat UX epic is related but not the canonical landing target.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Wait for the in-progress win-x64 and win-arm64 build jobs to complete before merge.
  • Use the recommended Mantis visual task if maintainers want proof beyond the copied UI diagnostic output.

Mantis proof suggestion
A visual task would materially show smooth large-chat scrolling and follow behavior, which source review and diagnostics cannot make fully visible. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify native chat timeline with a large conversation scrolls smoothly and preserves scroll-to-bottom/follow behavior after virtualization.

Risk before merge

  • [P1] The PR changes the row host underneath scroll-to-bottom, per-session offset restoration, history prepend, and tool-call expansion behavior; the focused UI proof covers a large append/follow path but not every production scroll-state path.
  • [P1] At inspection time the main test job had passed, but the x64 and arm64 build jobs for the current head were still in progress, so merge should wait for the final check rollup.

Maintainer options:

  1. Require scroll-state proof before merge (recommended)
    Ask for Mantis, computer-use, or equivalent current-head proof that a large native chat preserves follow, session restore, and history-prepend behavior after virtualization.
  2. Accept the focused UI diagnostic coverage
    Maintainers can merge on the current source review plus the 241-row ItemsRepeater UI diagnostic if they are comfortable owning unproven edge scroll states.
  3. Pause until the check rollup finishes
    Keep the PR open until the remaining Windows build jobs complete and any follow-up visual proof is available.

Next step before merge

  • [P2] No narrow automation repair is identified; maintainers should review the virtualized row host, wait for the check rollup, and decide whether the existing UI diagnostic proof is enough or a Mantis visual pass is needed.

Security
Cleared: No concrete security or supply-chain concern was found; the diff does not change dependencies, workflows, secrets, permissions, or artifact download paths.

Review details

Best possible solution:

Land the ItemsRepeater-based virtual timeline after current checks complete and maintainers are satisfied that large-chat scrolling, follow behavior, history prepend, and session offset restoration remain correct.

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

Not applicable; this is a feature/performance PR rather than a reproducible bug report. Source inspection confirms current main and v0.6.12 still render chat rows through VStack, while the linked backlog item defines the desired virtualization behavior without a failing perf threshold.

Is this the best way to solve the issue?

Yes, with review caveats: an ItemsRepeater-backed FunctionalUI virtual stack matches the existing A2UI virtualization direction and keeps the change localized. The safer merge path is to pair it with completed checks and proof for the scroll-state cases most likely to regress.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes copied current-head Windows UI diagnostic output for a 241-row native chat timeline using ItemsRepeater with the newest row visible, and GitHub shows the test job for that head succeeded.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body now includes copied current-head Windows UI diagnostic output for a 241-row native chat timeline using ItemsRepeater with the newest row visible, and GitHub shows the test job for that head succeeded.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a normal-priority chat UI performance improvement with limited blast radius, not an emergency or urgent regression.
  • merge-risk: 🚨 session-state: The diff changes the timeline row host under existing follow and per-session scroll offset state, which green unit tests alone do not fully settle.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The PR body now includes copied current-head Windows UI diagnostic output for a 241-row native chat timeline using ItemsRepeater with the newest row visible, and GitHub shows the test job for that head succeeded.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes copied current-head Windows UI diagnostic output for a 241-row native chat timeline using ItemsRepeater with the newest row visible, and GitHub shows the test job for that head succeeded.
Evidence reviewed

What I checked:

Likely related people:

  • RBrid: Introduced the native FunctionalUI chat experience including OpenClawChatTimeline and FunctionalUI, and later touched the same chat approval/timeline path. (role: introduced behavior and recent area contributor; confidence: high; commits: 08cab69a038c, 4e7982bafb86, 7d9152f427a3; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs, src/OpenClawTray.FunctionalUI/FunctionalUI.cs, src/OpenClaw.Tray.WinUI/Chat/OpenClawChatRoot.cs)
  • kenehong: Commit history shows repeated work in the chat timeline renderer around assistant footer usage, tool-card layout, and bubble behavior. (role: feature-history owner; confidence: high; commits: ff38d8499c48, 1287011c9f56, 6a5921883c86; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs)
  • shanselman: Recent current-main and GitHub history show work on chat bubble rendering and the tray UI test fixture used by this PR's proof test. (role: recent area contributor and merger; confidence: medium; commits: 4166e0fd63f8, 87dc3dc79d7c, d23f8ca50013; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatTimeline.cs, tests/OpenClaw.Tray.UITests/TestApp.cs)
  • TurboTheTurtle: Beyond this PR, GitHub history shows a merged multiple-chat-attachments change touching ChatRoot, ChatPage, ChatWindow, and related chat tests. (role: recent adjacent chat contributor; confidence: medium; commits: c5aa130dbf55; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatRoot.cs, src/OpenClaw.Tray.WinUI/Pages/ChatPage.xaml.cs, src/OpenClaw.Tray.WinUI/Windows/ChatWindow.xaml.cs)
  • codemonkeychris: Introduced the native WinUI A2UI renderer path that already uses ItemsRepeater virtualization, which is the closest existing renderer precedent. (role: adjacent renderer owner; confidence: medium; commits: 9fa43f347703; files: src/OpenClaw.Tray.WinUI/A2UI/Rendering/Renderers/ContainerRenderers.cs, tests/OpenClaw.Tray.UITests/A2UIControlMatrixTests.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works
  • 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.

@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. labels Jun 30, 2026
@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

@openclaw-mantis visual task: verify native chat timeline with a large conversation scrolls smoothly and preserves scroll-to-bottom/follow behavior after virtualization.

@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. merge-risk: 🚨 session-state 🚨 Merging this PR could lose, corrupt, stale, or mis-associate session or agent state. and removed 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. labels Jun 30, 2026
@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

Added current-head Windows CI proof to the PR body: the native chat timeline mounted 241 rows with ItemsRepeater, vertical StackLayout, bottom scroll offset, and newest row visible. @clawsweeper re-review

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed 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. labels Jun 30, 2026
@TurboTheTurtle

Copy link
Copy Markdown
Contributor Author

Current head ef95d1d has all GitHub Actions Build and Test jobs green, including test, e2e, win-x64, and win-arm64. The PR body includes current-head Windows CI proof: CHAT_TIMELINE_VIRTUALIZATION_PROOF rows=241 itemsRepeater=ItemsRepeater layout=Vertical scrollableHeight=19967.0 verticalOffset=19967.0 newestRowVisible=true. @clawsweeper re-review

@karkarl karkarl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested locally on ARM64 — the ScrollViewer is out of sync with the virtualized chat rows. After loading a conversation, the scroll position gets stuck in the middle of the chat history rather than anchoring to the latest messages at the bottom. Looks like the viewport offset isn't being reconciled with the virtualization panel's realized item range.

@shanselman

Copy link
Copy Markdown
Contributor

Thanks for pushing this forward — the shape is promising, and the 241-row proof is a useful start. I did a maintainer pass against current main after #917, and I don't think this is safe to merge yet at our 90% confidence bar.

The main blocker is the interaction with the newly merged FunctionalUI prune pass. ItemsRepeater realizes virtual row controls later through IElementFactory.GetElement(...), outside the normal root render walk. During the next root render, PruneUnvisitedPaths() only sees the virtual stack wrapper and repeater as visited, not the realized row paths, so it can detach/forget rows that the repeater still owns. That likely causes visible row churn/blanking and lines up with the ARM64 report that the ScrollViewer gets stuck mid-history.

Two related scroll/lifecycle issues should be fixed with that:

  • ConfigureVirtualStack assigns a fresh ItemsSource array and a fresh ItemTemplate on every render, which can force ItemsRepeater to reset/re-realize rows during normal chat churn.
  • Follow-to-bottom still uses a single ScrollableHeight read. With virtualized, variable-height chat rows, that extent can change after realization, so the scroll can land above the newest row.

Suggested path: make virtualized row ownership prune-aware (for example, a virtualized subtree/prefix that is not swept by the root prune until the item leaves the ItemsSource, or explicit row path lifecycle in RecycleElement), keep the template/source stable across renders, and anchor follow behavior by item/realization or by looping until the extent stabilizes. Then add proof with variable-height rows and a render-churn step on a current-main rebased branch; ARM64 proof would be especially valuable given the existing report.

So: great direction, but requesting changes rather than merging this one today.

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

Labels

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. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. 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.

Chat UI virtualization (perf on large conversations)

3 participants