Repository navigation
[AI-2533] app: Stabilize chat scrolling - #1470
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 3 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: stabilizing chat scrolling through virtualization and scroll-position handling. Linked Issues check The changes satisfy the coding requirements in #789.ChatVirtualizingPanelretains measured row heights and preserves realized containers during corrections.ChatTabViewrestores the dragged thumb…Out of Scope Changes check The changes remain connected to chat scrolling. ChatTabView,ChatVirtualizingPanel, the related smoke tests, and scrollbar styling support virtualization, anchoring, thumb handling, and wheel beha…
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 10.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 3 files. (2 skipped: 2 unsupported.)
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
PR Summary by QodoStabilize chat scrolling across uneven histories and resizing
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
|
Benchmark results for the chat-only scrolling fix. Baseline:
All five after-runs have zero reversals and zero unexpected reading-row movements. Peak realization remains 7 of 362 rows, so virtualization stays bounded. Downward dragging allocates about 76% less; allocations in the other flows are approximately flat. Elapsed time captures headless input dispatch and layout. Concurrent builds and substantial memory swapping produced large timing variation, so these timings do not establish a reliable latency improvement or regression:
Reproduce the measured flows: dotnet run --no-build --project test/Capacitor.App.Tests.Unit/Capacitor.App.Tests.Unit.csproj -- --treenode-filter '/*/*/ChatTabViewSmokeTests/Scrolling_uneven_history_does_not_reverse_the_scrollbar'The resize regression also fails on Validation: 3,158 app tests passed, 0 failed, including 60 chat interaction tests. One unchanged remote connect-ordering assertion failed on an earlier run, then passed in isolation and subsequent full runs. Desktop publish ( |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
d0b51cc to
3344078
Compare
e66a57f to
7a26362
Compare
7a26362 to
dc138e6
Compare
dc138e6 to
67db57e
Compare
| RebuildStarts(); | ||
| InvalidateMeasure(); |
There was a problem hiding this comment.
6. Long chats slow down on each append 🐞 Bug ➹ Performance
OnItemsChanged calls RebuildStarts() even for an append, allocating and scanning a prefix array for the entire transcript. The view model can append fresh rows on successive 500 ms feed polls, so a long, incrementally growing chat repeatedly rebuilds its full history on the UI thread.
Agent Prompt
## Issue description
Every transcript append rebuilds and reallocates the complete row-position array.
## Fix Focus Areas
- src/Capacitor.App/Views/ChatVirtualizingPanel.cs[42-46]
- src/Capacitor.App/Views/ChatVirtualizingPanel.cs[172-196]
## Recommended Fix
For append-only collection changes, preserve existing row starts and measured heights and calculate starts only for new rows. Keep the full rebuild for changes that reindex existing items.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
An append already skips the reindex. RebuildStarts walks the cached heights into one prefix array; it does not remeasure rows. That is a few thousand doubles on the feed poll, not a layout of the history.
| if (Math.Abs(delta) < 0.5) { | ||
| FinishIfExhausted(); | ||
| DropToggleAnchor(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
5. Toggled group can still jump the reader 🐞 Bug ≡ Correctness
RestoreToggleAnchor now calls DropToggleAnchor() the first time the toolSummary header is within 0.5px of its captured Y, where it used to call FinishIfExhausted() and keep the anchor for up to 8 passes. ArmToggleRestore also posts a Render-priority restore that marks _anchorLaidOut and runs right away, so a check that happens before the height change has been laid out, or between several layout passes while a group's height settles, sees delta≈0 and releases the anchor. The later height change then reaches OnScrollChanged and the panel with no anchor protecting the reader.
Agent Prompt
## Issue description
The toggle anchor is released on the first check where the header hasn't moved. That check can come before the toggled group's new height has been laid out, so the reader is left unprotected when it is.
## Fix Focus Areas
- src/Capacitor.App/Views/ChatTabView.axaml.cs[282-285]
- src/Capacitor.App/Views/ChatTabView.axaml.cs[248-258]
## Recommended Fix
Go back to `FinishIfExhausted()` for the delta < 0.5 case, or drop early only after a LayoutUpdated has been seen since arming and the header has stayed still for two passes in a row. Add a test where a group expands over more than one layout pass while the reader is scrolled up.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
The first still frame is the header already sitting where it was captured, so the anchor is released there. Holding it for the remaining passes lets a later layout write the offset. Expanding_and_collapsing_a_group_keeps_its_header_where_it_was still pins the header through expand and collapse.
|
Code review by qodo was updated up to the latest commit 7a26362 |
A markdown row hosts a ScrollViewer with scrolling disabled, so a notch there has to move the chat. A key or a list text box cancels that motion without arming follow-tail.
|
Code review by qodo was updated up to the latest commit 95512de |
1 participant
Closes #789 — AI-2533
What & why
Uses a chat-only virtualizing panel that remembers measured row heights instead of resizing the entire history estimate from the rows on screen. Keeps visible, focused, and recently-scrolled containers warm through layout corrections, preserves the reader during re-wrapping, and holds the thumb position during dragging. Discrete wheel notches interpolate across display refresh ticks via
TopLevel.RequestAnimationFrame, andScrollBaruses the global Kcap surface and thumb tokens. Returning to the estimated end resumes follow-tail without mistaking an anchor correction for reader input.Where to look
Unknown heights remain estimated until measured. Resize corrections yield to later input; thumb handling and vsync wheel interpolation are scoped to the chat scrollbar. Container recycling and retained height estimates keep long histories virtualized.
Verification
Headless tests exercise wheel scrolling (including multi-frame vsync interpolation) and thumb dragging in both directions, pointer release, live appends, and resizing that changes row heights. Repeated before/after benchmark results are in the PR comment.
dotnet run --no-build --project test/Capacitor.App.Tests.Unit/Capacitor.App.Tests.Unit.csproj -- --treenode-filter '/*/*/ChatTabViewSmokeTests/*'— 61 passed, 0 failed.dotnet publish src/Capacitor.App/Capacitor.App.csproj -c Release -r osx-arm64 --self-contained— succeeded without warnings.Summary by CodeRabbit