feat(ui): virtualize #chat-history to fix long-chat lag#4998
feat(ui): virtualize #chat-history to fix long-chat lag#4998touzenesmy wants to merge 6 commits into
Conversation
|
Heads up, I just found #4661 which overlaps with this. It caps #chat-history at 150 and pages older messages off to the server, plus adds Maybe it would be better if the virtualization is added to his commit ? |
4425ff1 to
af3af74
Compare
Five root causes addressed: 1. Unbounded DOM growth: _trimChatHistoryDOM caps #chat-history at 150 children. Old messages are offloaded and replaced with a clickable 'Show N older messages' bar that fetches pages from the server via the paginated /api/history endpoint. 2. Thinking-block O(n^2) rendering: during streaming, thinking text renders as plain textContent (O(1) per token). A single mdToHtml pass happens when the block closes. Full content always visible. 3. Document streaming layout thrash: textarea/code updates throttled to requestAnimationFrame. updateLineNumbers (which forces layout per line and creates DOM per line) skipped entirely during streaming — only runs once on finalize via syncHighlighting. 4. No server-side pagination: GET /api/history/:id accepts ?limit=&offset= and returns 'total'. Frontend requests ?limit=400. 5. Background stream memory leak: _purgeStaleBackgroundStreams frees accumulated text and abort controllers from completed/error entries. Bonus: content-visibility:auto on .msg, .agent-thread, .thinking-content so the browser skips paint/layout for off-screen elements.
…lder paging
The /api/history/{id} endpoint returns messages in chronological order
(oldest first), but the frontend's ?limit=400 call with offset=0 was
returning the oldest 400, not the most-recent 400 as the comment claimed.
- Backend: when no explicit offset query param is given, slice from the
end so the initial load returns the most-recent messages.
- Frontend: store data.total on initial load and update the "load older"
offset formula to count backward from the end of the array:
offset = total - 400 + _unloadedMsgCount - 50.
Add 6 tests for the ?limit / ?offset pagination in routes/history_routes.py to address the requested-changes blocker from odysseus-dev#4661 review: - No-offset ?limit=k returns most-recent k messages, not oldest - Explicit offset returns exact slice from chronological array - Adjacent explicit-offset windows are disjoint and contiguous - Most-recent window + explicit-offset backward page are adjacent, no gap - total excludes hidden messages (compaction summaries) - No ?limit returns all non-hidden messages
…ct tests Gold-standard audit of the fork's MessageWindow virtualizer against upstream (odysseus-dev#4644/odysseus-dev#4661/odysseus-dev#4998) and current best practice. Mechanical core (scroll anchoring, eviction geometry, teardown, race safety) already matched reference; the systematic weakness was accessibility. Fixes (each guard-tested): - aria-busy brackets prepend/prune batches (_loadOlder/_loadNewer) so AT coalesces churn instead of announcing scroll-up history as new; _clearBusy restores the PRIOR value so it composes with the streaming aria-busy chat.js toggles on the same log element rather than clearing it mid-stream. - focus inside an evicted node is moved to the log container, not dumped to <body> (_pruneTop/_evictLive/_loadOlder/_loadNewer). - interactive bottom sentinel gets role=button/tabindex/Enter+Space; decorative top sentinel and spacer get aria-hidden. - corrected the misleading "overflow-anchor:none globally" comment to the real invariant (only sentinels/spacer set none; Chromium target). Removed the dead upstream pager cluster orphaned on the fork by the olderLoader path (_installHistoryPager/_renderHistoryMessage/_addHistoryMessageWithFullRenderer + 3 now-unused helpers) — zero call sites, zero test refs. Tests: test_history_pagination_contract.py (pagination math incl. limit cap / offset clamp / has_more_after, hidden-row + inline-base64 stripping through the paginated branch, and a direct route-shadowing guard); test_chat_history_a11y_js.py (7 real-Chromium a11y guards); extended the real render-path Playwright test to seed markdown/fenced-code/image content, closing the markdownModule-regression class beyond plain strings. 161 chat/history tests green together. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…scape + audit Update the decision record and active-work to one current story: the merged upstream pager still has no bounded-DOM path (supersession holds), but two OPEN upstream PRs now circle this area — odysseus-dev#4661 (paging-only) and odysseus-dev#4998 (chatVirtualizer windowing that bounds lag, not RSS) — plus issue odysseus-dev#4644 requesting DOM removal. Records the honest two-axis verdict (ours wins on memory, odysseus-dev#4998 on simplicity/ state-preservation), the convergence path if odysseus-dev#4998 merges, CONTRIBUTING constraints (issue-first agent PRs, attach eviction to odysseus-dev#4644), and the gold- standard audit outcome (a11y fixes, dead-code removal, new tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Apples-to-apples Chromium benchmark of chat-history rendering strategies on the axes that matter — retained DOM nodes (memory) and append layout cost (lag) — over a history-length curve, so the result is reproducible-by-a-skeptic. Strategies use real code, no strawmen: naive (upstream merged-pager shape), detach (upstream PR odysseus-dev#4998 chatVirtualizer.js vendored verbatim), evict (the fork's MessageWindow). Fairness verified: odysseus-dev#4998 collapses 458/500 at n=500, so the arm exercises their real behaviour. Instrument selected by discriminator probe: CDP Performance.getMetrics.Nodes counts detached-but-referenced nodes, so it captures odysseus-dev#4998's retention — the honest structural memory metric. Critical caveat documented: JSHeapUsedSize does NOT include detached DOM (C++/Blink memory), so a performance.memory-only benchmark would wrongly exonerate odysseus-dev#4998; node count is primary, JS heap is JS-side retention only, and the fork's _all string retention is reported against us (it is never evicted — fairness cuts both ways). Generated result (medians of 3, n=100..2000): detach node count tracks naive exactly (871..15691 — odysseus-dev#4998 does not bound node memory); evict stays flat ~718. Append layout: naive/detach climb 3..35ms; evict flat ~2.5ms. Methodology, honest claims, and the specified next arms (hybrid dominance; scroll-back-vs-network trade; measureUAM DOM-bytes corroboration) in docs/dev/chat-history-benchmark.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…le-dependence Upgrade the benchmark to a defensible standard after audit: - Real byte metric: renderer-process USS/RSS via psutil (measureUAM verified unavailable in headless Chromium even when crossOriginIsolated). Per-cell fresh persistent context located by unique user-data-dir; max single renderer isolates the page's DOM cost. Node count kept as the structural proxy. - Statistical rigor: repeats with 1 warm-up discarded, median +/- (max-min) dispersion published per cell, raw per-run values retained in the JSON, environment (Chromium build/CPU/RAM/OS) captured. - Corrected claims the byte metric overturned: odysseus-dev#4998 DOES reduce memory (~15-30%, detaching frees render memory though node count is unchanged) and the win is scale-dependent (ours is slightly WORSE below ~1000 msgs; material only at 2000+). Node-count-only framing overstated the gap. - Fixed a misleading caption: the append-lag columns do NOT capture odysseus-dev#4998's benefit (appends hit the live region, not collapsed nodes); odysseus-dev#4998 targets scroll-repaint lag, which this harness does not yet measure. Recorded result (medians of 4, 1 warmup dropped) in results/bench.{json,md} with per-run raw values; e.g. USS@5000: naive 116.8 / detach 81.5 / evict 50.8 MB. The detach@5000 USS shows a flagged outlier (one run 119.4) — dispersion is published, not hidden; large-n cells need more repeats. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ering Adds an instrumented bottom->top scroll sweep (per-frame rAF intervals) so the lag axis odysseus-dev#4998 actually targets is measured, not assumed. Bumps repeats to 7 to tame the n=5000 USS variance seen at 4. Fixes the JS-heap table printing its median in MB and its spread in bytes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The convergence path this fork planned -- warm-band detach (odysseus-dev#4998) over cold-tail eviction -- was built, guard-tested, and benchmarked. It is strictly worse than eviction alone. The reason is structural, not a tuning miss: 1. detach-preserve retains a SUPERSET of what eviction retains over the same window (it keeps __vChildren), so eviction cannot lose to it on memory at any band size; 2. a live window already delivers odysseus-dev#4998's entire value proposition -- instant recent scroll-back -- at zero work and zero network. Any hybrid tuned toward that converges to eviction. Docs rewritten to state what was found rather than what was predicted. The arm and its five Chromium guard tests are kept, relabelled: a tested refutation with data is a result. Its known spacer-drift defect is fork issue #126; the harness withholds the affected cells rather than publishing them. Also records the probe failures that motivated the harness's guards. Each produced a plausible, flattering number for the favoured arm, and each died on inspection of the underlying record. Two rules follow: an extreme value means read the record, and never edit the harness while a run is in flight. Upstream posture unchanged except to simplify: file #125, attach eviction to odysseus-dev#4644, and offer odysseus-dev#4998's author the one thing we found for them -- their jank has a single root cause, offsetHeight read inside the IntersectionObserver callback, where entry.boundingClientRect.height is already free. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Fixes from 3-model review (Deepseek + Gemini 3.1 Pro + GPT-5.6 Sol): BLOCKERS: - Remove 666-line dead shim from routes/history_routes.py (sys.modules replacement bypassed the entire backend change) - Fix 6/6 failing pagination tests via temp SQLite DB fixture CRITICAL: - Enforce chat token scope in _verify_session_owner (non-chat tokens could access session history + mutations) HIGH: - Remove duplicate load-older pager, consolidate to scroll pager - Make document stream identity-safe (capturedId + rAF re-check) - Bound _backgroundStreams Map (MAX=10 + oldest-eviction) - Debounce thinking regex processing (200ms, avoids residual O(n^2)) - Configurable MAX_CHAT_DOM_NODES via data-max-dom-nodes attribute - Clean background-image data URIs in DOM trimmer Tests: 26/26 pass (6 pagination + 8 scope + 12 existing)
…tests The new scope check in _verify_session_owner requires API tokens to carry 'chat' or '*' scope. Two tests in test_session_owner_attribution.py created api_token=True requests without api_token_scopes, causing 403. Add api_token_scopes=['chat'] to: - test_bearer_owner_A_cannot_verify_owner_B_session - test_owner_can_verify_their_own_session
Real app.py under uvicorn, real /api/history over HTTP, real MessageWindow + olderLoader wiring driven by scroll; RTT emulated via CDP after app load. Result: deep scroll-back network cost is serialized pages x RTT, confirmed empirically at both lengths (n=500: +591ms at RTT 150 vs 4x150 predicted; n=2000: +2476ms vs 19x150). Server handler ~2ms/page; full 2000-message walk is 19 requests / 50 kB gzipped. odysseus-dev#4998's zero-network scroll-back is real but bounded by this slope; it does not dominate. The driver embeds two measured lessons: MessageWindow's _loadOlder fires from a one-shot IntersectionObserver, so a pinned-at-top hammer deadlocks (swallowed callback while _loading leaves the sentinel intersecting forever), and a down-up jiggle inside one frame is invisible to IO, which evaluates at frame boundaries. Both stall states are documented in the driver comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… Part 2's missing comparison) The bench compared naive/odysseus-dev#4998/evict/hybrid but never odysseus-dev#4661 itself -- the PR this contribution will be filed next to. Vendored faithfully from 27f35e1 (constants, removal loop, teardown, offset arithmetic all the PR's own; renderer/fetch/session adapted for the harness, marked). The arm's reload is CLICK-driven, so the scroll excursions legitimately stall for it (cells withheld, self-describing); its own click path is measured via loadOlderAll (wall time + clicks + post-reload DOM state). First smoke (n=500): steady-state DOM IS bounded (~150 children / nodes_loaded 1188) -- falsifying the record's 'odysseus-dev#4661 misses the memory bound' characterization, to be corrected with the full run -- while the click-reload path restores everything with nothing evicting behind it (nodes_peak 3991) until the next message re-trims. 5 guard tests pin vendor faithfulness. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r arms Compares four chat-history rendering strategies on renderer memory and layout cost, in real Chromium via CDP, at n = 250/1000/2000/5000 with 5 repeats. naive upstream's merged prepend-only pager -- never removes detach vendored PR odysseus-dev#4998 chatVirtualizer.js, verbatim with provenance evict DOM removal + server refetch (vendored MessageWindow snapshot) hybrid warm-band detach over cold-tail evict -- a hypothesis, refuted here Instrument crux: CDP Performance.getMetrics `Nodes` counts detached-but- referenced nodes, and renderer USS is read from the process. JSHeapUsedSize alone does NOT see odysseus-dev#4998's detached DOM (it lives in C++/Blink), so a perf.memory-only benchmark would have wrongly exonerated it. Result: DOM removal bounds retained nodes (~720 flat) where detach-preserve tracks the naive baseline (39091 at n=5000). The hybrid buys nothing over eviction alone -- eviction wins USS outside spread at n=1000/2000 and the two are within spread at n=250/5000. The refutation is structural, not a tuning miss: detach-preserve retains a superset of what eviction retains. Every arm's genuine win is reported, including two that cut against the conclusion: odysseus-dev#4998 beats eviction on recent scroll-back layout, and eviction's deep scroll-back excludes its server-refetch cost. Cells that would flatter an arm on work it never did are withheld and carry a machine-readable reason. Four measurement artifacts caught during development are documented with the guard that now prevents each. All three non-trivial arms are vendored snapshots, so this runs from a clean checkout of any branch. The evict arm is the fork's MessageWindow, NOT the sessions.js eviction staged upstream; that distinction is stated in the file header, the methodology doc, and the limitations section, because the shared mechanism -- not shared source -- is what these numbers measure. Verified: node --check on all vendored arms; py_compile; 6 passed 1 skipped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d6dea4e to
8982a14
Compare
Summary
Long chats get laggy and eventually freeze because every message stays in the DOM
forever and markdown/highlighting is re-run per message, so layout and paint cost
grows with the conversation. This adds a small, framework-free windowing layer
(
static/js/chatVirtualizer.js) over#chat-history: messages near the viewportstay live, and messages far off-screen have their children detached with their pixel
height pinned, then re-attached on scroll-back. Detach keeps the real nodes (not
innerHTML), so
<details>state, code highlighting and listeners survive, and thepinned height keeps scroll position stable. It never detaches a node that is actively
streaming (
.stream-content), theagent-thinking-dotsspinner, or the tail, soin-flight output is never lost. On a ~170 message chat this takes scrolling from
janky to flat (about 165/171 messages collapsed off-screen).
Note on scope: this fixes lag that grows with message count and lets big finished
bubbles drop out of the live DOM. A single very large block while it is still
streaming is still heavy to render; that overlaps with #4644 and is intentionally
out of scope here.
Target branch
dev, notmain.Linked Issue
Part of #2869 (also relevant to #4644)
Type of Change
Checklist
devwiring in
static/app.js, one test). No unrelated refactors.How to Test
docker compose uporuvicorn app:app).collapsedclimbs as messages leave the viewport and drops asyou return to them. Scrolling stays smooth and the scroll position does not jump.
normally and are never collapsed.
./venv/bin/python -m pytest tests/test_chat_virtualizer_js.py -v→ 6 passed.
Visual / UI changes
Functional only. It detaches/reattaches existing nodes and pins height; it adds no
colors, fonts, spacing, classes, components, or emoji.
Screenshots / clips