upgrade: mediastream migration and adaptation upgrade for Solid 2.0#919
upgrade: mediastream migration and adaptation upgrade for Solid 2.0#919davedbase wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: 875792c 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@packages/mediastream/README.md`:
- Line 136: Update the README line about createMediaPermissionRequest to clarify
that it forwards navigator.mediaDevices.getUserMedia behavior: the returned
Promise<void> resolves when permission is granted and rejects if the user denies
permission or getUserMedia fails (include both denial and other errors),
referencing createMediaPermissionRequest and navigator.mediaDevices.getUserMedia
so readers know the rejection semantics.
In `@packages/mediastream/src/index.ts`:
- Around line 83-86: The getUserMedia/getDisplayMedia promises in
createStream/createScreen (navigator.mediaDevices.getUserMedia and
navigator.mediaDevices.getDisplayMedia) lack rejection handling, causing
unhandled rejections and leaving a stale stopped stream in state; update the
calls in the functions that use setStream/stopStream (refer to setStream,
stopStream and the active flag) to catch errors (either add .catch(...) or
convert to async/await with try/catch), and on rejection ensure any previously
created stream is cleared by calling setStream(null/undefined) (and stopStream
on any obtained stream) and log or surface the error so the promise rejection is
handled.
- Around line 159-173: stop() currently disconnects audio nodes but doesn't
cancel the requestAnimationFrame loop, so calling stop() leaves loop() running;
update stop to call cancelAnimationFrame(rafId) (guarded if rafId is set) before
disconnecting/closing and ensure onCleanup either just calls stop() (remove the
separate cancelAnimationFrame(rafId) onCleanup) or keep it idempotent—use the
rafId symbol and the loop/stop functions to implement this change.
- Around line 82-89: The stream state isn't cleared when stopping or while
awaiting re-acquisition, so callers can get a stale stopped MediaStream; update
stopStream to call setStream(undefined) after stopping tracks, and modify
createStream and createScreen to clear the current stream immediately before
calling navigator.mediaDevices.getUserMedia/getDisplayMedia (e.g., call
setStream(undefined) right before the async call) so stream() returns undefined
while re-acquiring and after stopStream runs.
In `@packages/mediastream/test/setup.ts`:
- Around line 14-16: The analyser mock's getByteFrequencyData uses
Math.random(), causing flaky tests; update the function (getByteFrequencyData in
the test setup) to write a deterministic non-zero pattern instead (e.g., use
array.fill(128) or a fixed repeating sequence) so amplitude tests are stable and
still exercise non-zero data; ensure you replace the spread/map approach with
array.fill or an indexed assignment to avoid creating intermediate arrays.
🪄 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: 9b193b04-f2c1-4a17-a579-ce982c4e5547
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/mediastream-solid2-migration.mdpackages/mediastream/CHANGELOG.mdpackages/mediastream/README.mdpackages/mediastream/dev/index.tsxpackages/mediastream/package.jsonpackages/mediastream/src/index.tspackages/mediastream/test/index.test.tspackages/mediastream/test/server.test.tspackages/mediastream/test/setup.tspackages/mediastream/tsconfig.json
Replaces
@solid-primitives/streamwith a fully Solid 2.0–compatible package. Switches fromcreateResourceto reactive signals + splitcreateEffectfor async stream acquisition.isServernow imported from@solidjs/web. Return types simplified —mutate/refetchremoved; use source accessors for reactivity. Loading/error states handled via<Loading>and<Errored>from@solidjs/web.Design changes (Step 1)
createResourceis gone in Solid 2.0 → replaced withcreateSignal(INTERNAL_OPTIONS)+ splitcreateEffectfor async stream acquisitionactiveflag in the effect's apply phase prevents a supersededgetUserMediacall from overwriting a newer stream (race condition fix)createAmplitudeFromStreamcreateEffectconverted to split compute/apply formloop()starts synchronously insidecreateRoot, sosetAmplitudeneedsINTERNAL_OPTIONSto avoidSIGNAL_WRITE_IN_OWNED_SCOPEAPI changes
createStream/createScreen: return is now[Accessor<MediaStream | undefined>, { stop, mute }]—mutateandrefetchremoved (reactivity is source-driven)createAmplitudeStream: second element simplified to{ stream, stop }ResourceActionstype export removedSummary by CodeRabbit
New Features
@solid-primitives/mediastreampackage for Solid.js v2 with media stream utilitiesDocumentation