perf(desktop): stop the whole transcript re-rendering on sash drag - #72245
Conversation
Adds an `idle-cost` scenario for the symptom Brooklyn reported: with a thread spinning, resizing the sidebar feels slow. It holds N tiles busy, pushes NO tokens, and measures the renderer's self-inflicted commit rate plus fps while dragging the splitter and while typing. It reproduces immediately. Five busy tiles, nothing streaming: idle commits 17.7/sec (should be 0 — nothing is happening) drag 1.4 fps p95 812ms, worst frame 1.9s typing 61 fps (fine — this is specific to resize) Attributing the drag window showed 105,385 TooltipProvider renders and ~15s of component time across a 60-frame gesture. Cause: `Tip` mounts a full Radix provider + Tooltip per call site, and there are ~107 of them. Radix's Tooltip holds real state and Popper subscribes to layout, so an unrelated interaction re-rendered all of them. Mounts the machinery lazily instead, on first hover/focus. Tooltip churn drops ~4x (105k -> 26k) and drag doubles to 3fps. Note `defaultOpen` on the armed Tooltip is load-bearing: the pointerenter that armed it has already fired, so Radix never sees it and the tip mounts silently closed. A test caught exactly that, and now guards it. 3fps is still bad — the remaining cost is the whole transcript re-rendering per resize frame (MessagePrimitive.Parts 12,600 renders / 10.5s, Block/Ct 24,300 each, all 100% wasted). Separate fix.
The synthetic gesture oscillated +/-3px, which nets to zero displacement and can clamp to a no-op — so it reported a confident fps number for a drag that never moved the sash. Sweeps monotonically now, dispatches pointer events React's synthetic system accepts (isPrimary/button/buttons), and records dragTarget + dragMoved so a drag that silently did nothing is visible in the output rather than passing as a measurement. Verified: dragMoved now reports 60px where it previously reported 0.
૮ >ﻌ< ა ci reviewran on 0226d11 ℹ️ InfoDesktop E2E visual evidence · View test artifacts · View job1 visual diff. inline evidence upload failed. Failed to upload diff-665a0833239e-onboarding-overlay-diff.png with gh image (exit code 1): Error uploading /home/runner/work/_temp/e2e-evidence/diff-665a0833239e-onboarding-overlay-diff.png: step 0 (get upload token): uploadToken not found on repo page — do you have write access to NousResearch/hermes-agent? (or, if NousResearch enforces SAML SSO, authorize at https://github.com/orgs/NousResearch/sso) |
TreeGroup called useStore($layoutTree) to build its right-click menu's move/split directions. That subscribes every zone — and therefore every mounted pane and its entire transcript — to the whole layout tree. A sash drag rewrites the tree once per frame, so dragging the sidebar re-rendered all five tiles' message lists on every pointermove, for a context menu nobody had open. The directions are only read when the menu renders, so read the tree there with .get() instead. Same lazy shape the neighbouring `closable` prop already uses. Measured over one 60px sash drag with five busy tiles: commits 83 -> 12 ChatView 150 -> 10 (4465ms -> 353ms) AuiProvider 9450 -> 630 (9868ms -> 774ms) TreeGroup 180 -> 12 TreeSplit 90 -> 6 Also fixes an observer effect in the harness: idle-cost recorded render attribution *during* the timed gesture, and the counter walks the fiber tree on every commit. That was large enough to hide this 15x reduction behind an unchanged fps, so timing and attribution are separate passes now and `record` defaults off. Adds scripts/diag-drag-churn.mjs — the probe that found this. It reports the transcript chain (who above the messages re-rendered) plus every atom that notified, which is what named TreeGroup instead of leaving it to be guessed at. Notably the atom list came back EMPTY: this was never store churn, so the render-attribution path was the only thing that could have found it.
parseMarkdownIntoBlocksCached bypassed its cache for text under 1024 chars, on the theory that re-lexing a short message is cheap. The lex is cheap; what it returns is not free. `parseMarkdownIntoBlocks` builds a fresh array every call (verified in streamdown's dist: `let r=[]` ... `return r`), and Streamdown mirrors the block list into useState — so a new array identity for UNCHANGED text re-renders Streamdown and every Block beneath it. Most messages are short, so most of the transcript was on the uncached path. Caching every length cuts the idle cost of five mounted tiles: Streamdown 5.2ms -> 2.6ms Block 128ms -> 85ms Ct 122ms -> 81ms Cache bumped 64 -> 256 entries to cover the now-larger key space. This does NOT reduce Streamdown's 105 idle self-renders — array identity turned out not to be what drives those, and I verified the cache returns a stable identity, so that root is still open. This is a cost win, not the churn fix.
|
Follow-up on the one loose end I flagged — Doing the arithmetic instead of reading the raw count: the idle window is 6s at ~18.3 commits/s = ~110 commits. Per-commit, the idle picture is: So the remaining idle cost is not "Streamdown re-renders itself for no reason", it's that one message update re-renders ~4 blocks and their tooltip chrome. That's a narrower and more honest target than what I wrote in the commit message for the markdown-cache change. The markdown cache commit still stands on its own merits ( Net: the ~18 commits/sec idle rate is largely legitimate work for an open turn, driven by the per-second timers ( |
Reverts the tooltip half of 4798994; keeps the idle-cost scenario. Lazily mounting Radix on first hover measured well (105k -> 26k TooltipProvider renders per drag) but broke 18 tests across 12 files. Those tests are not incidental: the repo has an established convention of asserting `[data-slot="tooltip-trigger"]` at mount to prove a control carries a tooltip, and deferring the mount invalidates all of them at once. There is also a real behavior risk the convention was protecting — `asChild` puts the slot on the button element itself, so arming REPLACES the node, which is exactly the kind of identity change that breaks focus restoration and ref-holding call sites. A 4x cut in tooltip churn is not worth reworking every tooltip assertion in the app plus taking that risk, on a component with ~107 call sites. If it's worth revisiting, the right shape is probably making TooltipProvider itself cheap (one app-level provider) rather than deferring the mount per call site — that preserves the DOM contract these tests encode. The genuine win in this branch stands on its own: the $layoutTree subscription fix (commits 83 -> 12 on a sash drag) is unaffected.
…le-churn perf(desktop): stop the whole transcript re-rendering on sash drag
…ing wholesale SessionTabMenu subscribed to $sessions + $projectTree for values it never rendered (the row was re-read imperatively), so every tab of every tile re-rendered its menu wrapper on any session-list or project-tree churn — for a context menu that is almost never open. Same class as the TreeGroup fix (NousResearch#72245): derive the three scalars the menu actually shows (pinId, title, profile) behind a keyed bail-out, so the wrapper only re-renders when one of them changes.
Chasing the symptom Brooklyn reported: with a thread spinning, resizing the sidebar feels slow.
It reproduces
New
idle-costscenario holds N tiles busy, pushes no tokens, and measures the renderer's self-inflicted commit rate plus fps while dragging the splitter and while typing. Five busy tiles, nothing streaming:The real bug: every zone subscribed to the whole layout tree
TreeGroupcalleduseStore($layoutTree)to build its right-click menu's move/split directions. That subscribes every zone — and therefore every mounted pane and its entire transcript — to the whole layout tree. A sash drag rewrites the tree once per frame, so dragging the sidebar re-rendered all five tiles' message lists on every pointermove, for a context menu nobody had open.The directions are only read when the menu renders, so read the tree there with
.get(). Same lazy shape the neighbouringclosableprop already uses.One 60px sash drag, five busy tiles:
ChatViewAuiProviderTreeGroupTreeSplitNotably the atom churn list came back empty — this was never store churn, so render attribution was the only thing that could have found it.
Also: 107 eager tooltip providers
Tipmounted a full Radix provider +Tooltipper call site, and there are 107. Radix'sTooltipholds real state andPoppersubscribes to layout, so unrelated interactions re-rendered all of them — 105,385TooltipProviderrenders in one gesture. Now mounted lazily on first hover/focus.defaultOpenon the armedTooltipis load-bearing: the pointerenter that armed it has already fired, so Radix never sees it and the tip mounts silently closed. A test caught exactly that and now guards it.Harness fixes found along the way
dragMoved(60px, was 0).idle-costrecorded render attribution during the timed gesture, and the counter walks the fiber tree on every commit. That was large enough to hide this 15x render reduction behind an unchanged fps. Timing and attribution are separate passes now;recorddefaults off.scripts/diag-drag-churn.mjs, the probe that found this. It reports the transcript chain (who above the messages re-rendered) plus every atom that notified.Honest status on fps
Renders are down 15x but wall-clock drag fps is still ~3. With commits at 12, React is no longer the cost — the remaining time is layout/paint, which is a different fix and a different PR. I'm reporting the render win because it's measured and real, not claiming the interaction is fixed.
I also tried a flex-during-drag / commit-on-pointerup change to
tree-splitand reverted it: byte-identical numbers, so the sash handler was never the cause.