Virtualize and harden native chat timeline#949
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 9, 2026, 7:30 PM ET / 23:30 UTC. Summary Reproducibility: not applicable. as a PR: this is a native chat UI refactor/performance hardening change, not a standalone bug report. Source inspection, the PR body, and current-head checks identify the affected timeline, scroll, and permission paths. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Resolve the requested-changes review on the current head after maintainers confirm the native chat scroll/session-state proof, then land or keep the draft for a targeted follow-up. Do we have a high-confidence way to reproduce the issue? Not applicable as a PR: this is a native chat UI refactor/performance hardening change, not a standalone bug report. Source inspection, the PR body, and current-head checks identify the affected timeline, scroll, and permission paths. Is this the best way to solve the issue? Yes, subject to maintainer merge decision: the final delta fixes the requested permission label bug narrowly, and the native-host approach has targeted contract/UI proof tests. The remaining concern is manual acceptance of broad chat scroll/session-state behavior. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c8cd9869d828. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
Review history (5 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🦐 gold shrimp
clawsweeper
Bot
added
proof: sufficient
Add a clean-room virtualized chat timeline host backed by ItemsRepeater, preserve the existing chat visuals, and introduce the FunctionalUI native-control boundary needed to host native surfaces safely. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Split timeline and composer helpers into focused files, add a tested stable row-source synchronizer, and keep virtualization updates incremental without changing chat visuals. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Address final dual-review hardening items by removing per-render entry-id set allocation, stabilizing the thinking row key, locking session offset storage, preserving row host contexts across remounts, and reapplying prepend correction until virtualized extents stabilize. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Drop redundant OpenClaw prefixes from chat UI component and file names, and track realized virtualized rows explicitly instead of relying on ItemsRepeater parent semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Rebase the chat virtualization changes onto latest main, restore the extracted timeline helper, preserve synchronous first row measurement, and replace the recursive bottom chase with a single-shot ScrollViewer state machine that has fallback completion. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep the chat virtualization branch scoped to chat changes; the WorkspacePage work remains preserved in the local stash. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep Chat opening at latest unless the user intentionally left it scrolled up, restore timeline helper contracts after rebasing onto latest main, and use a single-shot bring-into-view state machine with synchronous first row measurement. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Render all gateway-provided permission actions, map AllowedAlways decisions to the correct label, and add contract coverage for the Allow always action and badge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adjust the UI proof test to validate the native VirtualizedChatView ItemsRepeater path, including the chrome spacer row and native-host realization metrics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Update the native timeline proof assertions, restore Allow always rendering, and keep chat-open scroll behavior aligned with the latest-row policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
bkudiess
force-pushed
the
chat-virtualized-refactor
branch
from
570d8ef to
3629e1b
Compare
shanselman
left a comment
shanselman
left a comment
There was a problem hiding this comment.
Thanks for pushing this forward — this is a big and valuable direction. Moving the native chat timeline to an ItemsRepeater/VirtualizedChatView path is the right kind of work for long conversations, and I appreciate that the PR keeps the existing visual design while adding tests around row identity, StableRowCollection, permission rendering, and the virtualization proof path.
I did a dual-model adversarial pass plus my own inspection. The broad architecture looks promising, but I do not think this is merge-ready yet. There is one concrete approval-label bug, and the exact-head test job is still pending, which matters a lot for a PR that touches the chat renderer and FunctionalUI native hosting.
Blocking before merge
Unknown permission actions render visually as "Allow"
In ChatTimeline.cs, pending permission actions are normalized and rendered like this:
var label = action switch
{
ChatPermissionActionKeys.AllowAlways => allowAlwaysLabel,
ChatPermissionActionKeys.Deny => denyLabel,
_ => allowLabel,
};
var button = Button(label, () => onResponse?.Invoke(requestId, action))That means any future/non-standard gateway action key is visually shown to the user as Allow, while the click sends the original unknown action key. Because this UI is a security/approval surface, the label must describe the action being submitted. A user should never click a button labeled "Allow" and actually send an unknown action.
Suggested fix:
- Keep the explicit labels for known actions:
allow/ allow-once → "Allow once"allow-always→ "Always allow"deny→ "Deny once" or the existing deny copy
- For unknown actions, render a safe explicit fallback rather than
allowLabel. Examples:- use a humanized action key:
Action: <key> - or a localized format:
Chat_Permission_ActionFallbackFormat = "Run action: {0}"
- use a humanized action key:
- Add a regression test that proves an unknown action key does not render as "Allow" but still invokes the original action key if clicked.
This should block merge.
Exact-head test check is still pending
At review time, gh pr checks 949 still showed the Build and Test test job pending. This PR changes:
- native chat rendering,
FunctionalUInative host behavior,- virtualization / realization,
- scroll-to-latest behavior,
- permission rendering,
- and UITest proof expectations.
For this kind of change, the test job is not just routine. It is the primary behavior proof for Tray tests, FunctionalUI tests, and the virtualization UI proof path. Please wait for it to finish green or rerun it if stuck.
Additional validation I would like before merge
Once the blocker above is fixed and CI is green, please include current-head proof for these specific behaviors:
-
Open at latest
- Open Chat with a non-empty history.
- Confirm the visible timeline starts at the latest message and the scroll-to-latest affordance is hidden when already following latest.
-
Preserve intentional scrolled-up position
- Scroll up intentionally.
- Navigate away and back, or otherwise force a remount if that is part of the intended contract.
- Confirm the scrolled-up position is preserved when it should be.
-
Load earlier / prepend anchoring
- Trigger the load-earlier path.
- Confirm prepended rows do not yank the user to the top or bottom unexpectedly.
-
Follow-bottom during streaming
- While following latest, stream/update an assistant response.
- Confirm the viewport follows smoothly and the scroll-to-latest button does not flash or incorrectly appear during programmatic animated scrolling.
-
Permission actions
- Confirm
Allow once,Always allow, andDenyrender correctly. - Confirm the decided badge for
AllowedAlwaysrenders as intended. - Include the new unknown-action fallback test mentioned above.
- Confirm
Non-blocking notes
FunctionalUI native host design looks mostly right
The NativeElement/ConfigureNative path appears to respect the existing FunctionalUI mark-and-sweep lifecycle because the native host itself is still created through the normal GetOrCreate<ContentControl>(path) path and therefore gets visited during the eager root render. The virtualization is then inside the hosted native control, and each realized row owns its own host/render path. That is the right pattern for adding virtualization without fighting the renderer's prune model.
Please keep this invariant in mind: lazy/virtualized controls must keep the owner host visited every render, and any realized row host needs an explicit cleanup path. The current direction seems aligned with that.
Re-mounting realized rows on every update may be acceptable but deserves attention
RefreshRealizedRows appears to refresh all realized rows whenever the backing row map changes. Because virtualization bounds the realized set, this is probably okay, and FunctionalUI diffing should make it tolerable. Still, streaming assistant text can update frequently, so please keep an eye on whether token streaming causes visible row churn or unnecessary work.
Source-text tests are useful but not sufficient
Several tests are source-contract tests that assert implementation shape. That is helpful for guarding refactors in WinUI-only seams, but the merge confidence for this PR should come from runtime behavior: FunctionalUI tests, Tray tests, and the UI virtualization proof tests. Please do not treat source-text assertions alone as proof that scrolling/realization behavior works.
Suggested done criteria
I would be comfortable re-reviewing once:
- unknown permission actions no longer render as "Allow";
- a regression test covers that behavior;
- the exact-head
testjob is green; - PR proof explicitly covers open-at-latest, scrolled-up preservation, load-earlier anchoring, follow-bottom during streaming, and permission action rendering.
Again, the overall direction is good. I just want to be careful because this PR changes the core chat rendering surface and an approval UI path.
Avoid showing unknown gateway permission action keys as Allow, add localized fallback copy, and cover the security-sensitive label behavior with a contract test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Addressed Scott's blocking permission-action feedback: unknown gateway permission actions now render with an explicit localized fallback label (\Action: ) instead of \Allow, while still submitting the original action key. Added contract coverage for the fallback, and full local validation is green (build, Shared, Tray, FunctionalUI). |
2 participants
Summary
ItemsRepeaterhost while preserving the existing chat visual designAllow alwayspermission action rendering andAllowedAlwaysdecided badge handlingAction: <key>instead of falling through toAllowVirtualizedChatView/ItemsRepeaterpathValidation proof
Validated locally on
chat-virtualized-refactorafter the latest push:Additional focused proof:
Manual / computer-use proof
openclaw://chat.VirtualizedChatViewhost.UI proof note
ChatTimelineVirtualizationProofTestswas updated to validate the new nativeVirtualizedChatView/ItemsRepeaterpath, including the chrome row and native-host realization expectations.Package dependency criteria could not be resolved), so CI remains the source of truth for the UITest job.Notes