new: video package for Solid 2.0#920
Open
davedbase wants to merge 9 commits into
Open
Conversation
🦋 Changeset detectedLatest commit: 0510607 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
davedbase
changed the title
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://ghub.bbstale.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/video-initial.md:
- Line 15: The changeset incorrectly documents fullscreen controls that were
intentionally excluded; update the text to remove any mention of fullscreen
methods and controls: remove `requestFullscreen`, `exitFullscreen`,
`toggleFullscreen` from the `makeVideoPlayer` description and remove
`fullscreen`/fullscreen controls from the `createVideoPlayer` description so the
changeset matches the PR objectives and README (references: makeVideoPlayer,
createVideoPlayer).
In `@packages/video/dev/index.tsx`:
- Line 2: The demo imports createVideo but calls methods that belong to
createVideoPlayer; either change the import to import createVideoPlayer from
"../src/index.js" or modify the demo to only use the createVideo API surface
(playing/setPlaying, currentTime/seek, ended, seeking, error, duration). Locate
usages that call setMuted, toggleFullscreen/fullscreen, volume, playbackRate,
readyState, videoWidth/videoHeight, setVolume, setPlaybackRate and replace them
with the corresponding createVideoPlayer calls (if available) or remove/replace
those interactions to use createVideo’s exposed properties (playing, setPlaying,
currentTime/seek, ended, seeking, error, duration).
In `@packages/video/src/createVideo.ts`:
- Line 93: The duration is currently set on the player's loadeddata event which
may fire after metadata is available; update the event wiring in createVideo so
you call setRawDuration(player.duration) on the player's loadedmetadata event
instead of loadeddata (look for the loadeddata: () =>
setRawDuration(player.duration) entry and change it to use loadedmetadata) so
duration initialization occurs as soon as metadata is ready.
In `@packages/video/src/createVideoPlayer.ts`:
- Line 13: The documentation and examples for createVideoPlayer incorrectly
reference createVideoControls and fullscreen methods that are not part of
createVideoPlayer’s public API; update all doc strings/examples (including the
other occurrences noted) to remove mentions of createVideoControls and any
fullscreen methods and instead list only the actual returned properties such as
buffered, readyState, videoWidth, and videoHeight (and any other real members on
createVideoPlayer’s return value). Ensure examples show usage of
createVideoPlayer alone and its real methods/properties and do not imply it
exposes fullscreen controls or a createVideoControls helper.
In `@packages/video/src/make.ts`:
- Around line 66-67: Update the JSDoc for the makeVideo wrapper to remove the
claim about providing fullscreen controls and instead state it only provides
playback controls and exposes the player for external fullscreen integration;
specifically edit the comment above the makeVideo (or make) function to mention
"playback controls and `player` for external fullscreen handling" and that
fullscreen must be implemented by the consumer rather than provided by this
primitive.
- Around line 5-10: The setVideoSrc function only sets either el.src or
el.srcObject, which can leave the other field populated and cause the wrong
media to remain active; update setVideoSrc to always clear the alternative
property before assigning the new source (e.g., when setting a string src, set
el.srcObject = null first; when setting a MediaProvider/stream, clear el.src =
"" before assigning el.srcObject) so both HTMLVideoElement.src and srcObject are
consistently managed.
In `@packages/video/src/types.ts`:
- Around line 51-53: Update the JSDoc on the exported API to accurately describe
createVideoPlayer and the actual readiness event: change references from
`createVideoControls` and `loadeddata` to `createVideoPlayer` and metadata
readiness (e.g., `loadedmetadata` or "metadata loaded"), state that it throws
`NotReadyError` until metadata has loaded (integrates with <Loading>), returns
duration in seconds reactively after metadata loads, and resets to pending when
the source changes; update the same wording for the other JSDoc block mentioned.
In `@packages/video/test/setup.ts`:
- Around line 26-53: createMockState is missing the currentTime, src, srcObject
and buffered mock fields; add these properties to the VideoMock returned by
createMockState (e.g., currentTime: 0, src: "", srcObject: null, buffered: {
length: 0, start: () => 0, end: () => 0 } or a simple TimeRanges mock) and then
define prototype accessors on global.HTMLVideoElement.prototype for
"currentTime" (getter/setter that read/write this._mock.currentTime), "src"
(getter/setter -> this._mock.src), "srcObject" (getter/setter ->
this._mock.srcObject) and "buffered" (getter -> this._mock.buffered) so tests
using player.currentTime, player.src, player.srcObject and video.buffered() read
from the mock; reference createMockState, the video._mock fields, and
global.HTMLVideoElement.prototype when making the changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b8caa5a9-d280-4214-8124-85a5ec537bd7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
.changeset/video-initial.mdpackages/video/LICENSEpackages/video/README.mdpackages/video/dev/index.tsxpackages/video/package.jsonpackages/video/src/createVideo.tspackages/video/src/createVideoPlayer.tspackages/video/src/index.tspackages/video/src/make.tspackages/video/src/types.tspackages/video/test/index.test.tspackages/video/test/server.test.tspackages/video/test/setup.tspackages/video/tsconfig.json
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.
@solid-primitives/video— layered primitives for HTML video playback, built for Solid 2.0 (beta.14)makeVideo→makeVideoPlayer→createVideo→createVideoPlayer@solid-primitives/fullscreenhandles it;video.playeris exposed for that integrationPrimitives
makeVideo— creates a rawHTMLVideoElement, binds optional event handlers and initial config (autoPlay,loop,muted,preload). Returns[player, cleanup]. No Solid owner required.makeVideoPlayer— wrapsmakeVideowith imperative controls:play,pause,seek,setVolume,setMuted,setPlaybackRate,setLoop. Returns[controls, cleanup]. No Solid owner required.createVideo— reactive primitive overmakeVideo. Tracks essential playback state:playing/setPlaying,currentTime/seek,ended,seeking,error.durationthrowsNotReadyErroruntil metadata loads, integrating with<Loading>.createVideoPlayer— extendscreateVideowith the full reactive control surface:volume/setVolume,muted/setMuted,playbackRate/setPlaybackRate,loop/setLoop,buffered,readyState,videoWidth,videoHeight.Summary by CodeRabbit
New Features
@solid-primitives/videopackage for HTML video playback integration with SolidJS.Tests