Uh oh!
There was an error while loading. Please reload this page.
fix(ui): stop unrelated re-renders from rewriting tooltip anchors - #2033
Closed
Astro-Han wants to merge 4 commits into
Closed
fix(ui): stop unrelated re-renders from rewriting tooltip anchors#2033Astro-Han wants to merge 4 commits into
Astro-Han wants to merge 4 commits into
Conversation
`@astryxdesign/core`'s `useLayer` built the context-mode trigger ref from a fresh inline closure each render and returned a fresh layer object besides. `useTooltip` composes both into its own ref, and `Tooltip`'s layout effect depends on that ref, so every render of every tooltip on the page tore the effect down and re-ran it — unconditionally rewriting `aria-describedby` and the inline `anchor-name` on triggers whose state had not changed. Over one streamed answer that measured as 58080 attribute writes against untouched history, with frame rate falling from 118fps to 22fps. Patch the dependency to hold both identities stable, keeping `isOpen` in the memo dependencies so a real open or close still propagates, and guard it with a test that renders the real `Tooltip` into a write-recording DOM. Refs #2030
…tity Review of the `useLayer` fix found what stabilizing the ref exposes: `Tooltip`'s layout effects depend on `tooltip.ref` and never on the element they actually read, so they re-attach when React hands them a new callback and never when the trigger itself changes. Upstream is correct only by accident — the ref changed every render, so every render happened to rescan. Against a stable ref the same code misses a trigger that is replaced or arrives late, leaving the new element unattached and the detached one still carrying an anchor-name. No call site hits this today; all seven wrap one stable IconButton. Key attachment on the element instead: one layout effect, no dependency array, returning immediately unless the resolved trigger differs from the one held. That is strictly stronger than the ref fix alone — it also drops the rewrite on open and close — and it makes the documented "Children refs are preserved" true for a changing child. The guard now pins both directions. Only asserting that nothing is written when nothing moved is what let this through. Refs #2030
…the closure
Shrinks the patch to the half that carries the fix, and corrects two defects
the element-keyed rewrite introduced.
The `useLayer` half is gone. `usePopover` returns a fresh object literal per
render and `Popover`'s attach effects depend on that object as a whole, so
memoizing `useLayer` never reached the `DropdownMenu` path; measured against a
real session it contributed nothing to the write count the element-keyed
`Tooltip` half already takes to zero. It did change effect-firing timing for
`Carousel`, `Tokenizer`, and `ContextMenu` — components this repo neither uses
nor tests. The stray `dist/Layer/useLayer.js.orig`, a `patch` tool leftover that
`patch-package` captured as a new file and that made up 47% of the diff, is gone
with it. The patch is regenerated from a clean tree: 4 files to 2, 836 lines to
401.
Attachment is now split by what each piece of state is actually about.
`positionRef` (inline `anchor-name`) and `aria-describedby` describe the
element, are idempotent, and stay keyed on element identity — this is the entire
cost, and it stays at zero. The event listeners close over current props and can
only be removed with the exact function that added them, so they are keyed on
that closure and detached through it. Keying them on the element instead froze
the mounting render's closure: `isEnabled={false}` stopped suppressing hover,
and unmount called `removeEventListener` with identities that never matched,
leaking listeners onto a trigger the tooltip does not own.
Detach now removes only this tooltip's own id from `aria-describedby` rather
than restoring an attach-time snapshot, which goes stale as soon as a second
tooltip shares the trigger or the app writes to the attribute.
Letting the listeners rebind every render is upstream's own behaviour and is
measured, not assumed: in Chromium against a transcript-shaped DOM, 145200
listener operations cost 11.0ms across a whole streamed answer with 0.0ms of
style or layout, against 29.1ms (21.2ms of it style recalc) for half as many
attribute writes.
The guard grows from 4 cases to 13 and now runs against the mechanism it is
testing. `useIsomorphicLayoutEffect` resolves `useLayoutEffect` vs `useEffect`
once, at module evaluation, from `typeof window`; the hoisted static import made
every previous run exercise `useEffect`, not the layout effect the patch relies
on. The fake DOM also recorded no listeners at all, leaving the entire
hover/focus path unreachable. Both are fixed, and mutation testing confirms the
suite kills reverting the patch, freezing the listener closure, dropping the
listener bind, dropping the unmount detach, ignoring `anchorRef` mode, and
restoring an aria snapshot.
Refs #2030…nchor by element The interaction half kept its own `boundRef` bookkeeping so it could remove listeners through the closure that added them. React already guarantees that: mutation-phase cleanup for a render runs before the next render's effect body, so a per-render `return () => detach(null)` closes over exactly the binding closure. The bookkeeping is deleted. Two correctness holes the guard did not reach: - Detaching through `tooltip.positionRef(null)` removes the anchor name from whatever element `useLayer` currently holds, not from the element this tooltip attached to. When element children become text children, the text-only span's JSX ref has already moved `useLayer`'s trigger, so the detach stripped the anchor off the element that just acquired it. Anchor names are now added and removed directly with `tooltip.anchorId`. - `aria-describedby` is a value invariant, not an element-identity one. An application rewriting the attribute from its own props dropped the tooltip's id permanently, because the element had not moved and the effect skipped. The unchanged-element path now re-reads the attribute and re-asserts its own id; reading is not a write, so the zero-write steady state is unchanged. Guard grows 13 -> 16 cases and records what the fake DOM deliberately does not model. `patches/README.md` corrects the `useLayer` blast radius (`useKeyboardHint` backs TabList/Toolbar/SegmentedControl, and DropdownMenuSubMenu calls useLayer directly), and marks the rebind benchmark as a one-off measurement whose script is not in the repo.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR A of #2030. A streamed answer drops the desktop app from 118fps to 22fps. One contributing layer sits in
@astryxdesign/core: every unrelated re-render rewrote DOM attributes on tooltip triggers that had not changed.Tooltipattached its trigger from two layout effects that listedtooltip.refas a dependency and never listed the element they actually read (anchorRef.current, orwrapper.firstElementChild).useTooltipcomposestooltip.reffromuseLayer's context ref, which is a fresh inline closure per render, so both effects tore down and re-ran on every render of every tooltip on the page. Each run unconditionally executesremoveAttribute/setAttribute('aria-describedby')andremoveAnchorName/addAnchorName— and the latter pair writes inlinestyle.anchor-name, which dirties style. The cost is O(every tooltip trigger on the page) per unrelated parent re-render.What this actually buys, in the A+B world
Every one of those writes lands on footer buttons belonging to history, not to the turn being streamed:
chat-turn.tsx:590-597renders the live turn's footer as anaria-hiddenplaceholder<div>with noTooltipin it. So the write count is a product of two factors, and this PR only moves one of them:This PR is therefore not "a fix for 44608 writes". It makes any re-render that legitimately happens free at the trigger, and it keeps being worth having after PR B lands, because PR B reduces the count rather than guaranteeing zero.
Measured, CDP, same real session and same stress each time
useLayerhalf onlyScope note: this CDP run predates the shrink and its third column is the two-half patch. The 0-writes result carries over unchanged — it is produced entirely by the
Tooltiphalf, and the unit guard proves the element-keyed effect writes nothing on an unrelated re-render withuseLayerpristine (four-way table below). The only behavioural delta the shrink introduces is per-render listener rebinding, which cannot dirty style or layout at all (see below). The frame-time column has not been re-measured against the shrunk patch.The fix: key each piece of state on what it is actually about
A tooltip puts two different kinds of state on its trigger, and upstream keyed both on
tooltip.ref:The inline
anchor-nameandaria-describedbydescribe the element. They are bookkept against element identity and skipped when the trigger has not moved. This is the entire cost.interactionRefinstalls event listeners closing over current props (isEnabled,delay,hideDelay,focusTrigger,onOpenChange).removeEventListenermatches only the exact function that was added, so these are bound and released by React's own per-render effect cleanup:React runs the cleanup a render created before the next render's effect body, so
detachis always the very closure that added the listeners. An earlier revision maintained aboundRef{el, detach}record to guarantee the same thing by hand; that is React's guarantee, not this component's job, and the bookkeeping is gone.Upstream was correct only by accident, and in both directions at once: because
tooltip.refchanged every render, every render happened to rescan the DOM (correct, and the whole cost) and happened to rebind listeners with fresh closures (correct, and free of style/layout work). Fixing only one half breaks the other:useLayerhalf onlyTooltiphalf only (this PR)An element-keyed attach is not simply idempotent, and three separate corrections say why:
aria-describedbyinstead of restoring the attach-time snapshot. A snapshot goes stale the moment anything else writes to the attribute — two tooltips sharing a trigger, or the app updating it mid-mount — and restoring it un-describes the other writer or resurrects a value the app already replaced.aria-describedbyis a value invariant, not an element-identity invariant. With the element unchanged, an early return skips a trigger whosearia-describedbythe application rewrote from its own props: React writes the new value, this tooltip's id is gone, and nothing puts it back — the description breaks permanently and does not self-heal. The unchanged-element path now re-reads the attribute and re-asserts its own id when it is missing. Reading is not a write, so the zero-write steady state is untouched.useLayer's ref.layer.ref(null)removes the anchor name from whatever elementuseLayeritself last held (useLayer.js:215) — not necessarily the element this tooltip attached to. When element children are replaced by text children, the text-only<span>'s JSX ref has already pointeduseLayerat the span by the time the layout effect runs, solayer.ref(null)stripped the anchor off the element that had just acquired it. The patch now adds and removes the anchor name directly viaaddAnchorName/removeAnchorNamewithtooltip.anchorId, which is the half oflayer.refthat belongs to this component;useLayer'striggerRefis used for nothing else.Plus the change that makes them reachable: the trigger-resolving effect runs with no dependency array and returns immediately unless the resolved element differs from the one it holds, which makes
TooltipProps' documented "Children refs are preserved" true for a child that is replaced (<button>→<a>), arrives late (null→<button>), or turns into text.Why the patch shrank, and why the listeners may rebind every render
The previous revision also memoized
useLayer's context ref and returned object. That half is removed:DropdownMenupath it was supposed to help.usePopover(Popover/usePopover.js:113,199) does calluseLayer, but returns a fresh object literal per render, andPopover's attach effects (Popover.js:222,251) depend onpopoveras a whole.Tooltiphalf alone already reaches 0 writes (column 3 above vs the four-way table).useLayerdirectly would change effect-firing timing:Carousel,Tokenizer,ContextMenu,useHoverCard,usePopover— but alsoDropdownMenu/DropdownMenuSubMenu.jsandhooks/useKeyboardHint.js, anduseKeyboardHintbacksTabList,ToolbarandSegmentedControl. This repo references those at 19, 16 and 20 sites respectively, withDropdownMenuat 34. The earlier claim that only components "this repo neither uses nor tests" were affected was wrong, and the correction argues for the shrink, not against it.Dropping it also drops a stray
dist/Layer/useLayer.js.orig(392 lines, 47% of the old diff) that apatchtool left behind andpatch-packagecaptured as a new file. The patch is regenerated from a clean tree, verified file-by-file against a freshnpm pack @astryxdesign/core@0.2.0: 4 files → 2, 836 lines → 431, and nothing innode_modules/@astryxdesign/corediffers from the published tarball exceptdist/Tooltip/Tooltip.jsandsrc/Tooltip/Tooltip.tsx.Without that half,
interactionRefchanges identity every render, so listeners unbind and rebind every render — exactly what upstream does. The reason that is affordable is an invariant, not a number:addEventListenerandremoveEventListenertouch no style or layout state, so they cannot dirty the cascade however many of them run, whileanchor-nameis an inline style write that always does. A one-off Chromium measurement (22 history turns × 6 tooltip'd buttons, 110 frames ≈ 2420 turn re-renders) put the rebinds at ~11.0ms mutation / 0.0ms style+layout per streamed answer against 7.9ms / 21.2ms for the attribute writes they replace. That harness was run once and its script is not in this repo;patches/README.mdnow labels it as a one-off illustration rather than a maintained benchmark, because the conclusion does not depend on it.Popover.js:222,251has the same defect — worse, in fact:attachTriggerrewritesaria-haspopup,aria-expandedandaria-controlsplus an inline style on every render, a superset of whatTooltipdid. It is deliberately not fixed here: nothing in the CDP profile points at it, and it is not on the streaming hot path, so fixing it would widen the patch surface without evidence.patches/README.mdrecords the reason to exist, the deletion condition as an executable procedure (bump, delete the patch, rebuild@maka/ui, run the named guard, keep it deleted only if all 16 cases stay green), whyPopoveris left alone, and what the test harness deliberately does not model. The previously documented "inlineonOpenChange" trap was wrong and has been removed: measured with the full patch, an inline arrow produces zero attribute writes on the trigger. Its real cost is toggle-listener churn on the popover element — a different mechanism, orders of magnitude smaller, and no call site passesonOpenChangeat all.This PR touches nothing in the transcript-derivation path;
materialize.tsandchat-view.tsxbelong to PR B.Refs #2030
Verification
npm run typecheck(workspace-wide),npm run format:check,npm run lint— clean.packages/ui: 284/284 pass, including the 16-case guard below.apps/desktopmain suite: 1544/1544 pass.npm ci: patch re-applies through the root postinstall, no.origor.rejanywhere in the tree,node_modules/@astryxdesign/corematches the published tarball except the two patched files, guard still green.npm test— this is a dependency patch plus one test file.The guard was validating the wrong thing; both faults are fixed
packages/ui/src/__tests__/tooltip-anchor-stability.test.tsxgrew from 4 cases to 16, and two harness faults meant the old 4 were not exercising the patch's mechanism:useEffect, notuseLayoutEffect.useIsomorphicLayoutEffectresolves once, at module evaluation, fromtypeof window !== 'undefined'. The hoisted staticimport {Tooltip}evaluated before the fake DOM was installed, so it bounduseEffect. Verified directly: window absent at import →=== useEffectistrue; window present →=== useLayoutEffectistrue. The patch's mechanism is a dependency-array-free layout effect. Fixed by installing the window in abeforehook and importing dynamically (module-scope top-levelawaitloses the race —node:teststarts the first test as soon as evaluation yields).FakeElement.addEventListener/removeEventListenerwere empty, making the whole hover/focus/press path unreachable. They now record(type, handler)pairs, which is the only thingremoveEventListenermatches on, so a cleanup passing a mismatched identity leaks in the fake exactly as it would in a browser.What the fake DOM still does not model is now recorded in the test next to
class FakeElement, so the gaps are visible rather than assumed away: noshowPopover/hidePopover(souseLayer.show()always takes the Safari<17style.displayfallback and neither the native popover path nor itstogglelistener is reached), a no-opdocument.addEventListener(so Escape-to-dismiss, WCAG 1.4.13, is unreachable), amatches(':focus-visible')that always answers true (so the focus gate is never verified in the negative), and notabIndexproperty (soisFocusablesees a text-only<span tabIndex={0}>as unfocusable and binds 3 of the 5 listeners a browser would). None of them touch what the patch changes: which element is written to, and which closure removes the listeners.Mutation testing
Every mutant below was actually injected into
node_modules/@astryxdesign/core/dist/Tooltip/Tooltip.jsand the guard re-run. Counts are the tests that went red.tooltip.positionRef(null)aria-describedbyre-assertionanchorRefsibling modeThe earlier table's first row said "no-write ×2, controlled open, shared-trigger detach" and read as four items; the actual count against the 13-case guard was three tests, and against the 16-case guard it is five. Counted per test from here on.
Two mutants are equivalent and disclosed rather than chased:
tooltip.positionRef(target)instead ofaddAnchorName(target, anchorId).useLayer's ref does exactlyremoveAnchorName(previousTrigger)+addAnchorName(el), and after the detach fix the previous trigger is already clear, so the two are observationally identical on the attach path.addAnchorNameis kept because it is the half that belongs to this component, and because routing through the layer ref is what made the detach path wrong.textOnlyterm from the trigger resolution. In text-only mode the<span>carriestooltip.refdirectly andwrapperRefis never attached, sowrapperRef.current?.firstElementChild ?? nullis alreadynull. Kept to mirror upstream's stated intent, not because a test guards it.The previous revision's
bound.el !== targetcheck was also equivalent (interactionRefchanges identity every render, so the closure check was unconditionally true). It no longer exists: React's per-render cleanup replaced the wholeboundRefrecord.Repo-wide survey: 13 production
Tooltipcall sites, all element children, 0 text-only, 0anchorRef, 0onOpenChange, 0 carrying their ownaria-describedby. The element→text and application-rewrites-aria-describedbydefects were therefore latent here, not live bugs — they are fixed because the patch is the thing that introduced the element keying that made them possible, and because both are cheap.