FE-1181: Fix entity slide-over jitter from scroll-lock feedback loop#8995
Conversation
…t scroll-lock time useScrollLock live-tracked the locked element's scrollbar size with a ResizeObserver. While the lock itself hides the scrollbar, that measurement reads 0, which released the lock, which restored the scrollbar, which measured non-zero and re-applied the lock — an infinite loop toggling the document scrollbar every frame whenever hiding it changed the body's box by any sub-pixel amount (fractional device-pixel-ratio scrollbar widths vs the integer compensation). The entity editor slide stack is fixed-positioned at right: 0, so it visibly shook by a scrollbar width at ~30Hz. Measure the scrollbar once when the lock is applied instead: the effect no longer depends on a value its own side effect changes, so there is no feedback path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxtJJuAiNutazSAFv4msbA
PR SummaryMedium Risk Overview
Adds an internal Reviewed by Cursor Bugbot for commit 8f925c6. Bugbot is set up for automated code reviews on this repo. Configure here. |
…measurement Move the hook from apps/hash-frontend/src/shared/use-scroll-lock.ts to libs/@hashintel/ds-components/src/util/use-scroll-lock.ts, next to useAvoidScrollWidthChange, and export it from the package entrypoint. Extract the duplicated scrollbar measurement into a shared src/util/scrollbar-size.ts: the element-level "offsetWidth - clientWidth - borders" formula (per axis) and the document-level "window.innerWidth - documentElement.clientWidth" variant, consumed by both hooks. The hooks themselves stay separate on purpose: useScrollLock measures once at lock time (the oscillation fix), while useAvoidScrollWidthChange live-tracks; useAvoidScrollWidthChange's behaviour is unchanged. Update the three hash-frontend call sites to import from @hashintel/ds-components and add a changeset for the package. Requested by Alex Leon in review of the oscillation fix. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxtJJuAiNutazSAFv4msbA
alex-e-leon
left a comment
There was a problem hiding this comment.
LGTM - Once I've done a round of manual testing I'll merge
Requested by Alex Leon · Slack thread
🌟 What is the purpose of this PR?
Before: with the entity editor slide-over open (e.g. on
/entities), the whole panel — contents and its own scrollbar — vibrates side-to-side by ~15px (a scrollbar width) continuously, at roughly every frame, while the page behind it stays still.After: the panel stays still.
How: the shake is the document scrollbar being toggled every frame by a feedback loop inside our own
useScrollLockhook (previouslyapps/hash-frontend/src/shared/use-scroll-lock.ts) — not MUI, and not the entities-table height measurement. The slide stack callsuseScrollLockwhen a slide opens (slide-stack.tsx:126), which hides the always-on document scrollbar (globals.scsssetsbody { overflow-y: scroll }) and compensates with an integerpadding-right. The hook also live-tracked the scrollbar size with aResizeObserveronbody, and its layout effect depended on that tracked value:overflow: hidden; padding-right: 15px→ scrollbar disappears → the slide (position: fixed; right: 0) shifts right by the scrollbar width, following the viewport edge.window.innerWidth - clientWidthcompensation is), hiding the bar changesbody's content box by a sub-pixel amount → the observer fires → it measures the scrollbar size while the lock has hidden the scrollbar, reads0→ the effect re-runs and its cleanup releases the lock even though it is still active.15→ the lock re-applies → loop, indefinitely at ~30Hz.The page behind doesn't move because the padding compensation keeps in-flow content constant to within a sub-pixel; only the viewport-anchored fixed slide follows the toggling scrollbar. On exact integer geometry the loop can't sustain (the body box doesn't change), which is why it only reproduces on some machines/scale factors.
The fix measures the scrollbar size once, synchronously, at the moment the lock is applied, and drops the live tracking entirely — the effect no longer depends on a value that its own side effect changes, so the feedback path is gone. This is the same principle documented in
useAvoidScrollWidthChange(libs/@hashintel/ds-components): never trust a scrollbar measurement taken while the bar is absent. Per review, the hook now lives in@hashintel/ds-components(src/util/use-scroll-lock.ts, next touseAvoidScrollWidthChange), with the scrollbar measurement both hooks share extracted into asrc/util/scrollbar-size.tsutil.Note: the working hypothesis was a regression from #8783/#8786 interacting with the #8660 measurement approach. The evidence contradicts that: in reproduction, the #8660
ResizeObserverfires once per toggle butcontentTopnever changes, so it is not part of the loop. The latent bug has been inuseScrollLockall along; the recent PRs merely changed the page on which people happened to have the slide-over open.🔗 Related links
use-scroll-lock.ts, theslide-stack.tsxfixed-position slide, andbody { overflow-y: scroll }: at--force-device-scale-factor=1.5the slide'sgetBoundingClientRect().xalternates512 ↔ 496.67(15.33px = the real fractional scrollbar width) with 60 flips per 120 frames; with the fix, 0 flips at scale factors 1, 1.5 and 2, with the lock still applied and held.🚫 Blocked by
Nothing.
🔍 What does this change?
useScrollLockmoved fromapps/hash-frontend/src/shared/use-scroll-lock.tstolibs/@hashintel/ds-components/src/util/use-scroll-lock.ts(next touseAvoidScrollWidthChange) and is exported from@hashintel/ds-components, per review.useLastScrollbarSize(theResizeObserver+ state that the effect depended on). The layout effect now measures the scrollbar size whenactivebecomes true and applies/removes the lock only onactive/elementToLockchanges. Behaviour is otherwise preserved: same measurement method (MUI'sgetScrollbarSizeapproach), still a no-op when the scrollbar size is 0 or when something else already setoverflow: hidden.libs/@hashintel/ds-components/src/util/scrollbar-size.ts: the scrollbar measurement previously duplicated in both hooks — the element-level "offsetWidth - clientWidthminus borders" formula (per axis:getConsumedScrollbarWidth/getConsumedScrollbarHeight) and the document-levelwindow.innerWidth - documentElement.clientWidthvariant (getDocumentScrollbarSize). BothuseScrollLockanduseAvoidScrollWidthChangenow consume it;useAvoidScrollWidthChange's behaviour is unchanged. The two hooks stay separate — measure-once-at-lock-time vs live-tracking is an intentional distinction.src/pages/shared/slide-stack.tsx,src/components/grid/utils/override-custom-renderers.tsx,src/pages/shared/block-collection/shared/mention-suggester.tsx) to importuseScrollLockfrom@hashintel/ds-components.@hashintel/ds-components(patch).Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
🐾 Next steps
scrollbar-gutter: stableon the root as a belt-and-braces way to make document-scrollbar toggling layout-neutral everywhere, which would also stop the slide-over's one-time ~15px shift into the vacated gutter when it opens.Drawer/useAvoidScrollWidthChangemachinery when it is next reworked (useScrollLockitself now lives in ds-components as of this PR).🛡 What tests cover this?
❓ How to test this?
--force-device-scale-factor=1.5reproduces it deterministically) with classic (non-overlay) scrollbars./entitiesand click a row to open the entity editor slide-over.📹 Demo
See the screen recording in the Slack thread for the before state.
🤖 Generated with Claude Code
https://claude.ai/code/session_01JxtJJuAiNutazSAFv4msbA
Generated by Claude Code