refactor(scrollbar): tighten ScrollbarComponent internals#8694
Merged
willeastcott merged 4 commits intomainfrom May 6, 2026
Merged
refactor(scrollbar): tighten ScrollbarComponent internals#8694willeastcott merged 4 commits intomainfrom
willeastcott merged 4 commits intomainfrom
Conversation
Small follow-up cleanup to #8693: - Declare _handleDragHelper as a class field (was implicit-via-assignment). - Tighten the handleEntity setter: resolve the candidate entity first, then a single early-return covers both "same Entity by ref" and "same GUID by string" cases. Switch || null to ?? null. - Drop the _onSetHandleAlignment passthrough; bind _updateHandlePositionAndSize directly to set:anchor / set:margin / set:pivot events on the handle element. - Use an early return in _updateHandlePositionAndSize. - _destroyDragHelper now uses optional chaining and nulls the field after destroy, so a post-element-loss enabled toggle no longer writes to a torn-down helper. - Inline the _setHandleDraggingEnabled helper into onEnable / onDisable. No public API changes. The 16 existing scrollbar tests still pass. Co-authored-by: Cursor <cursoragent@cursor.com>
willeastcott
added
enhancement
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up refactor of ScrollbarComponent internals to simplify handle wiring and improve drag-helper lifecycle handling after the larger architecture alignment work in #8693.
Changes:
- Declares
_handleDragHelperas an explicit private class field and improves_destroyDragHelper()cleanup. - Simplifies
handleEntitysetter by resolving the candidate entity up-front and using a single equality early-return. - Removes
_onSetHandleAlignmentpassthrough and binds_updateHandlePositionAndSizedirectly to handle element alignment-related events; minor simplification inside_updateHandlePositionAndSize.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… drag helper ElementDragHelper defaults to enabled = true in its constructor, so a helper created in _onHandleElementGain (e.g. when an element is added to the handle entity after the scrollbar is already disabled) would start out draggable even though onDisable had already run. Mirror the component's current enabled state on the new helper to keep the lifecycle consistent. Adds a regression test that exercises the late-arriving-element path on a disabled scrollbar. Co-authored-by: Cursor <cursoragent@cursor.com>
ElementDragHelper captures its constraint axis at construction time, so flipping the scrollbar orientation while a handle element exists left the helper still constraining drags to the old axis. _onHandleDrag would then read the wrong component of the position vector and value updates could stop working until the helper was destroyed and rebuilt through some other path (e.g. handleEntity reassignment). Extract the helper-rebuild logic from _onHandleElementGain into a shared _rebuildDragHelper method, and call it (plus _updateHandlePositionAndSize) from the orientation setter when there is an active handle element. Adds a regression test that flips the orientation post-attach and verifies the drag helper is rebuilt for the new axis. Reported by Copilot in #8693. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/framework/components/scrollbar/component.js:360
onRemove()only destroys the drag helper, but the component can also have active subscriptions on the current handle entity / handle element (_evtHandleEntityElementAddand_evtHandleEntityChanges). If the scrollbar component is removed at runtime, these listeners will keep the component alive and can still invoke callbacks after removal. Consider calling_handleEntityUnsubscribe()(and optionally clearing_handleEntity) inonRemove()to fully detach.
onRemove() {
this._destroyDragHelper();
}
…nent The handleEntity setter's JSDoc claimed the handle "must have a ScrollbarComponent", but the code relies on the handle's element (ElementDragHelper is constructed from handleEntity.element). Update the doc to require an ElementComponent, with useInput: true for the handle to be draggable. Reported by Copilot in #8694. Co-authored-by: Cursor <cursoragent@cursor.com>
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 free
to 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
Small follow-up cleanup to #8693, polishing the ScrollbarComponent internals while the file is fresh, plus two correctness fixes for
ElementDragHelperlifecycle bugs that Copilot flagged on #8693 and on this PR's first commit.Cleanup
_handleDragHelperas a class field (@type {ElementDragHelper|null}, defaultnull). It was previously implicit-via-assignment in_onHandleElementGain, the only piece of state on the component without a class-level declaration.handleEntitysetter: resolve the candidate entity first, then a single early-return covers both "same Entity by ref" and "same GUID-string by ref" cases. Switch|| nullto?? null._onSetHandleAlignmentpassthrough - bind_updateHandlePositionAndSizedirectly to theset:anchor/set:margin/set:pivotevents on the handle element._updateHandlePositionAndSize, drop thehandleElementlocal._destroyDragHelpernow uses optional chaining and nulls the field after destroy._setHandleDraggingEnabledhelper intoonEnable/onDisable.Bug fixes
ElementDragHelperbuilt in_onHandleElementGainnow mirrors the component's currentenabledstate, instead of inheriting the helper's defaulttrue. Previously, attaching a handle (or its element appearing) to an already-disabled scrollbar produced a draggable helper.orientationsetter now rebuilds the drag helper (and refreshes the handle position / size) when an element is already attached.ElementDragHelpercaptures its axis at construction, so without this, flipping orientation at runtime left drags constrained to the old axis and could stopvalueupdates from happening.Both bug fixes have regression tests.
Public API
No additions, removals, or behaviour changes.
Test plan
npm run lintpassesnpm run build:typesregenerates.d.tscleanly;npm run test:typespassesnpm test- 1690 passing, 0 failing (18 ScrollbarComponent tests, including 2 new regression tests for the bug fixes above)