fix(react): project slider state per render - #2595
Draft
mihar-22 wants to merge 1 commit into
Draft
Conversation
Slider.Root, TimeSlider.Root and VolumeSlider.Root kept a component- lifetime core in useState and wrote each render's props, media and input into it during render, so a speculative or abandoned render could leave the committed slider callbacks reading another render's values. Build the projection core per render instead, route useSlider's retained binder callbacks through useCommittedRef so only committed renders reach them, and keep the genuinely retained pieces (pointer and drag state in createSlider, the pause-on-drag intent, the wheel handler and the press based controls lock) commit-synchronized. Edge alignment now re-applies committed CSS vars from a layout effect instead of forcing a re-render. Refs #1679 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@videojs/core
@videojs/element
@videojs/html
@videojs/media
@videojs/react
@videojs/spf
@videojs/store
@videojs/utils
commit: |
Contributor
📦 Bundle Size Report🎨 @videojs/html — no changesPresets (7)
Media (18)
Extensions (2)
Players (5)
Skins (29)
UI Components (50)
⚛️ @videojs/react — 25 small size changes
Presets (7)
Media (22)
Extensions (2)
Players (5)
Skins (26)
UI Components (39)
🧩 @videojs/core — no changesEntries (76)
🏷️ @videojs/element — no changesEntries (2)
📦 @videojs/store — no changesEntries (3)
🔧 @videojs/utils — no changesEntries (13)
📦 @videojs/media
Entries (23)
📦 @videojs/spf — no changesEntries (10)
ℹ️ How to interpretEach entry is independently bundled, minified, and brotli-compressed. Initial size includes its static import graph; lazy dynamic chunks are reported separately. Entries are not additive because their dependency graphs overlap. Preset rows represent realistic combined bundles. Changes of 300 B or less across initial, lazy, and total size are collapsed, not discarded. Run |
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
1 participant
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.
Refs #1679
Depends on #2594.
Summary
#2589 left
SliderCore,TimeSliderCoreandVolumeSliderCoreretained becauseuseSlider's callbacks mutate them andTimeSliderCorecarries drag intent. This finishes that conversion without touching the publicuseSlider,Slider.Rootcallback, orcomputeStatecontracts: the render-derived projection (props, media, locale, ARIA, value mapping, CSS vars) is now a render-local core built per render, while the interaction machinery stays retained and reads only committed values.Changes
Slider.Root,TimeSlider.Root,VolumeSlider.Root: build the core per render (new *Core(props)) instead ofuseState(() => new *Core())plussetProps/setFormatLocalein render.computeStatestill callssetInput/setMediaon that core, but the core now belongs to the render that created it, so no other render can observe the mutation. ThesyncStylessubscription re-projects through the committed render'scomputeStatefor CSS var updates that bypass React, exactly as before.useSlider:optionsRefis auseCommittedRef, so the retainedcreateSliderbinder only ever reaches committed callbacks and cores;adjustPercentis read through the ref at call time instead of being captured from the first render (the captured version would have pinned the first render's core). The mount-timeuseForceRenderlayout effect is gone: edge alignment is applied bysyncStyles()from a layout effect on mount and wheneverthumbAlignmentchanges, which also covers a render that adjusted percents through the previously committed options.TimeSlider.Root: the render-local core projects props and media; a separate retaineddragCoreowns the pause-on-drag intent, withpauseOnDragcommitted from a layout effect andstartDrag/endDrag(including the unmount resume) driven from committed refs.mediaRefis replaced by the committed options closure.VolumeSlider.Root: the retainedcreateWheelStephandler reads the committed core, volume and disabled state throughuseCommittedRef(previouslygetStepPercentcaptured the first render's core).core.adjustPercentForAlignmentandslider.adjustForAlignment.What remains retained and why
createSlider(SliderApi): pointer capture, drag threshold, throttling, keyboard stepping and theinputstate store are interaction state that must outlive renders; its callbacks read committed options throughuseCommittedRef.dragCore(TimeSliderCore) inTimeSlider.Root: only holds whether a drag paused playback soendDragcan resume it (also on unmount). Its single input,pauseOnDrag, is committed in a layout effect rather than written during render.createWheelStepinVolumeSlider.Root: a listener attached through a callback ref; reads committed refs only.onPressStart/onPressEndinuseSlider) is unchanged.slider.input.currenton each render, soaria-valuenow/aria-valuetextfollow the pointer whenever the root re-renders during a drag (covered by a new test). Splitting motion from semantic state (refactor(react)!: separate slider render and motion state #2327) remains out of scope.Notes
VolumeSliderCore.getWheelStepPercent()returnsNaNbecauseSliderCore.setPropsusesdefaults(props, SliderCore.defaultProps), which dropswheelStep. Both the React and HTML volume sliders callsetVolume(NaN)on wheel. Worth a separate core fix with a core test.Testing
slider.test.tsx: an abandoned re-render insideMockErrorBoundarydoes not change what the retained slider projects (getPercent,isDisabled) or whichonValueChangeit calls; StrictMode replay projects the latest committed props into ARIA, CSS vars and callbacks; dragging updatesdata-draggingand thumb ARIA and value callbacks map through the latest committed range; the controls lock is held across re-renders during a press; switchingthumbAlignmenttoedgeapplies adjusted CSS vars in the same commit.time-slider.test.tsx: thumb announces the pointer position while dragging;onValueCommitseeks through the committed media; abandoned re-render keeps the committed projection; existingpauseOnDragtests coverdragCore, including unmount resume.volume-slider.test.tsx: committed step/disabled projection, abandoned re-render, and wheel disabled handling under StrictMode.pnpm -F @videojs/react test: 76 files, 596 tests passedpnpm build --filter=@videojs/reactpnpm typecheckpnpm lint:fix:fileon touched files andgit diff --check🤖 Generated with Claude Code