refactor(scrollview): align ScrollViewComponent with CameraComponent architecture#8777
Merged
Conversation
…architecture Apply the same refactor pattern recently adopted by ScrollbarComponent (#8693): the component now owns its state directly via private fields, setters do their work inline, the data-bag round-trip and set_<name> event chain are gone, and the system declares `_schema = ['enabled']` with an explicit `_properties` list and `cloneComponent`. `mouseWheelSensitivity` previously relied on the schema's `type: 'vec2'` auto-conversion (clones Vec2 inputs, builds a Vec2 from `[x, y]` arrays); that work is moved into the setter itself so JSON-loaded scenes shipping `mouseWheelSensitivity: [1, 1]` and callers passing shared Vec2 instances continue to work. Add `test/framework/components/scroll-view/component.test.mjs` covering defaults, the round-trip via `addComponent`, the Vec2/array normalization on `mouseWheelSensitivity`, side effects on `horizontal`/`vertical`, `set:scroll`, all four entity setters (Entity / GUID / null / unsubscribe-on-reassign), `cloneComponent`, and `resolveDuplicatedEntityReferenceProperties`. No public API change: properties, behavior, and the `set:scroll` event are preserved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors ScrollViewComponent to match the “component owns its state” architecture used by CameraComponent / ScrollbarComponent, reducing reliance on *ComponentData property bags while preserving the existing public API surface. It also adds a dedicated test suite to guard against regressions from the schema reduction (notably mouseWheelSensitivity normalization and cloning behavior).
Changes:
- Refactor
ScrollViewComponentto store most properties on private fields and remove the_setValue/set_*listener plumbing. - Reduce
ScrollViewComponentDatatoenabledonly, and updateScrollViewComponentSystemto use an explicit_propertieslist plus a customcloneComponent. - Add
test/framework/components/scroll-view/component.test.mjsto cover defaults, property round-trips, entity reference behaviors, cloning/remapping, andmouseWheelSensitivitynormalization.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/framework/components/scroll-view/component.test.mjs | Adds coverage for the refactored ScrollView component (defaults, setters, events, cloning, ref remap). |
| src/framework/components/scroll-view/system.js | Updates the system to schema ['enabled'], initializes via _properties, and adds explicit cloning + accessor build. |
| src/framework/components/scroll-view/data.js | Shrinks data class to enabled only. |
| src/framework/components/scroll-view/component.js | Moves property storage to private fields, inlines side-effects, and adjusts lifecycle/event handle management. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… on remove Two follow-ups from Copilot's review on #8777: - `mouseWheelSensitivity` setter was narrower than the old schema-driven `convertValue('vec2')`: it only normalized `Array.isArray(arg)`, so a Float32Array (or any other indexable input) would slip through to the passthrough branch and break later code that reads `.x`/`.y`. Match the old contract: nullish passes through, Vec2 is cloned, anything else is treated as indexable. - `onRemove` only detached the entity's own `element:add` and element listeners. Listeners registered on referenced viewport / content / scrollbar entities, plus the pending `_evtElementRemove` once-handle on the element, stayed live - keeping the removed component reachable from those entities. Set every entity ref to null (which routes through the existing setter -> unsubscribe path) and `.off()` the pending once-handle. This is also a pre-existing leak, not a regression from the refactor. Adds tests for both: a Float32Array round-trip on `mouseWheelSensitivity`, and a `removeComponent` test asserting that referenced entities no longer have any of the component's listeners. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review on #8777 caught a behavior regression: the old initializeComponentData normalized dragThreshold / useMouseWheel / mouseWheelSensitivity when they were `=== undefined`, so callers shipping `{ dragThreshold: undefined }` still ended up with the default 10. The refactored loop guarded with `hasOwnProperty`, which let explicit undefined values clobber the class-field defaults - turning `useMouseWheel` falsy (disables wheel scrolling) and `mouseWheelSensitivity` into undefined (`_onMouseWheel` then throws on `.x`/`.y`). Tighten the guard to `data[property] !== undefined` so the class-field defaults survive explicit-undefined input, exactly matching the legacy behavior. Adds a test that addComponent with `{ dragThreshold: undefined, useMouseWheel: undefined, mouseWheelSensitivity: undefined }` lands on the documented defaults. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.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
Applies the same refactor pattern adopted by
ScrollbarComponentin #8693 toScrollViewComponent, continuing the move away from*ComponentDataproperty bags.ScrollViewComponentnow owns_horizontal,_vertical,_scrollMode,_bounceAmount,_friction,_dragThreshold,_useMouseWheel,_mouseWheelSensitivity,_horizontalScrollbarVisibility,_verticalScrollbarVisibilitydirectly. Setters write the private fields and (forhorizontal/vertical) inline the_syncScrollbarEnabledStateside effect that used to live inset_horizontal/set_verticallisteners. The_setValuehelper,_toggleLifecycleListeners,_onSetHorizontalScrollingEnabled, and_onSetVerticalScrollingEnabledare gone. The four entity setters drop their trailingthis.data.<name> = guidwrites.ScrollViewComponentDatais reduced to a singleenabledfield.ScrollViewComponentSystemmatchesCameraComponentSystem/ScrollbarComponentSystem:_schema = ['enabled'], an explicit_propertieslist drivesinitializeComponentData, an explicitcloneComponentis added (the default base-class clone would otherwise lose every non-enabledfield), andComponent._buildAccessors(ScrollViewComponent.prototype, _schema)handles theenabledaccessor.The schema previously declared
mouseWheelSensitivityastype: 'vec2', which madeComponentSystem.convertValuecloneVec2inputs and construct aVec2from[x, y]arrays. With the schema reduced to['enabled'], that normalization is moved into themouseWheelSensitivitysetter so JSON-loaded scenes shippingmouseWheelSensitivity: [1, 1]and callers passing sharedVec2instances (e.g.Vec2.ONE) continue to work.The constructor's
entity.on('element:add', ...)registration is captured to a new_evtElementAddevent-handle field and torn down explicitly inonRemove. The pre-existing missing_evtViewportEntityElementAddfield declaration is added alongside the other event-handle fields for consistency.Public API
No additions, removals, or behaviour changes. The full property surface of
ScrollViewComponentis preserved (verified against the regenerated.d.ts):enabled,horizontal,vertical,scrollMode,bounceAmount,friction,dragThreshold,useMouseWheel,mouseWheelSensitivity,horizontalScrollbarVisibility,verticalScrollbarVisibility,viewportEntity,contentEntity,horizontalScrollbarEntity,verticalScrollbarEntity,scroll, and the staticEVENT_SETSCROLL/set:scrollevent.Tests
Adds
test/framework/components/scroll-view/component.test.mjs, modelled on theScrollbarComponenttest suite, covering:#addComponent— defaults match the data class; round-trip every property passed via the data argument.#mouseWheelSensitivity— accepts[x, y]arrays (the regression bait of dropping the schema entry); clonesVec2inputs so caller mutations do not leak into component state.#horizontal/#vertical— flipping the flag syncs the attached scrollbar entity's enabled state; same-value writes are no-ops.#scroll— assigning aVec2firesset:scrollexactly once.#viewportEntity,#contentEntity,#horizontalScrollbarEntity,#verticalScrollbarEntity— acceptEntity, accept a GUID string and resolve viaapp.getEntityFromIndex, acceptnull, unsubscribe from the previous entity when reassigned.#cloneComponent— round-trips every scalar property; remaps every entity ref to the cloned subtree.resolveDuplicatedEntityReferenceProperties— remaps every entity ref through the suppliedduplicatedIdsMap.Test plan
npm run lintpassesnpm run build:typesregenerates.d.tscleanly (public property surface diff-clean)npm test— all scroll-view tests pass; the only failing test (Http #get() retries resource and returns 404) is pre-existing onmainand unrelated to this refactor🤖 Generated with Claude Code