Skip to content

fix(core): stop config mutation and propagate native errors reliably - #5097

Merged
mziolkowskii merged 14 commits into
masterfrom
fix/core-player-errors-and-source
Oct 8, 2026
Merged

mziolkowskii merged 14 commits into
masterfrom
fix/core-player-errors-and-source

Conversation

@mziolkowskii

@mziolkowskii mziolkowskii commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes four bugs in the library's JS core that the new unit tests surfaced: createSource mutating the caller's config, errors swallowed after the last onError listener is removed, promises that hang or reject with undefined on a native rejection, and a second release() inside the grace window re-running teardown. Adds the unit tests that cover them.

Split out of #5087; the unit test tooling it relies on landed with #5096.

Motivation

  • createSourceFromVideoConfig wrote its defaults (initializeOnCreation: true, the platform DRM type, normalized externalSubtitles) back into the object it was given. useVideoPlayer keys the player on JSON.stringify(source), so a config the caller keeps around (a module constant, a useMemo/useState value) produced a different key on the next render and useManagedInstance destroyed and recreated the player.
  • triggerJSEvent returned true for an event whose last listener had been removed (the Set still existed, empty), so throwError treated the error as handled: after unsubscribing from onError, play()/pause()/seek*() failures vanished silently. This is a regression from refactor: events logic #4798, which dropped the listener count that Handle multiples event listeners #4708 had added.
  • wrapPromise re-threw from inside its own .catch when nobody listened to onError, so the outer promise never settled: await player.initialize() on a native rejection hung forever and logged an unhandled rejection. With a listener it rejected with undefined.
  • release() called twice inside the 5 s grace window ran __destroy again: native release() is idempotent on both platforms, but the JS side cleared events again, called native release() again and restarted the grace timer that keeps the native object reachable.

Changes

  • sourceFactory.ts: build the native config as a new object instead of mutating the input.
  • VideoPlayerEventsBase.ts: report "nobody listening" when the listener set is empty.
  • VideoPlayer.ts: one reportError path (parse, notify onError, return the parsed error) shared by the synchronous throwError and by wrapLoad, which runs the native load itself so that a synchronous throw (replaceSourceAsync with an invalid source, a released player) is reported like a native rejection; the promise always rejects with the parsed error. Loads carry a generation counter: a cancellation whose load was superseded by a newer load from JS only rejects, any other cancellation (the native player was released, e.g. by replaceSourceAsync(null)) is reported through onError like every other failure. __destroy is guarded by a released flag.
  • VideoError.ts: player/cancelled and source/cancelled added to the code unions; both platforms already emit them.
  • Unit tests: source normalization, event routing, VideoPlayer error propagation (including both cancellation cases) and release, useVideoPlayer; shared native mocks now record created players and keep emitter listeners.
  • Docs and the agent skill (references/v7/events.md): the async methods reject as well as call onError; the synchronous ones keep the "callback instead of throw" contract; the rules are stated for iOS and Android, web has its own VideoPlayer and reports through onError with web/* codes.

Behavior changes

Ticked as a behavior change below. What apps see differently when something fails:

  1. With an onError listener, initialize(), preload() and replaceSourceAsync() now reject with the error in addition to calling onError. Previously they rejected with undefined, so code that awaits them without a catch already saw an unhandled rejection; it now carries a VideoRuntimeError. Without a listener they used to hang; they now reject.
  2. A load superseded by a newer initialize()/preload()/replaceSourceAsync() from JS no longer reaches onError as player/cancelled; it only rejects. A user switching sources mid-load is not a playback failure. A cancellation with no newer load, which both platforms produce once the native player has been released, still reaches onError as before.
  3. Errors thrown after the last onError listener is removed are thrown again instead of dropped.
  4. Two error codes are added to the exported unions: player/cancelled, source/cancelled.

Interplay with #5135 (async onError from native): no double delivery. On both platforms the load promises reject and never emit the native onError (HybridVideoPlayer.kt / HybridVideoPlayer.swift document this), so a rejected initialize() reaches onError once. The E2E error flows assert exactly one onError and are green on this branch.

Platforms affected

JS core of the native players. The web player (VideoPlayer.web.ts) has its own implementation and is not touched.

  • Android
  • iOS
  • visionOS
  • tvOS / Android TV
  • Windows
  • Web

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behavior)
  • Documentation only

Test plan

  • bun run test: 251 tests pass (40 new library tests in this PR). bun typecheck and bun lint pass.
  • Against the library source on master, 12 of the new tests fail: the empty-listener event trigger and the throw-again case, the three promise propagation cases, the two cancellation cases, the two replaceSourceAsync cases, the release case, the config mutation guard and the two useVideoPlayer recreate cases.
  • The E2E gate (Android 0.77/0.82/0.87, iOS 0.87) is green on this branch. The flows only cover success paths and the asynchronous 404/broken-manifest errors; the promise and listener paths are covered by the unit tests, which is the right level for them.
  • The player recreate path is inferred from useVideoPlayer/useManagedInstance and covered by the hook tests with mocked native modules; it was not reproduced in a rendered app.
  • No new Maestro flow: the E2E test app sets initializeOnCreation explicitly and keeps its onError listener attached, so none of these paths are reachable from the current scenarios.

Merge order

#5098 no longer touches CONTRIBUTING.md, so the two PRs merge cleanly in either order.

Checklist

  • I read the contributing guidelines.
  • I added or updated tests that cover this change, or explained under "Test plan" why none apply.
  • For a bug fix the E2E suite can exercise, this PR includes a Maestro flow that fails without the fix.
  • bun run test, bun lint and bun typecheck pass locally.
  • I updated the documentation / README where relevant.
  • If this changes how the library is used (API, props, events, behavior), I updated the AI agent skill (skills/react-native-video/) to match.
  • This PR is focused on a single concern.
  • I tested my changes on at least one platform.

Okelm pushed a commit that referenced this pull request Sep 16, 2026
… RN 0.87 (#5096)

Adds a deterministic end-to-end harness and the CI around it, plus the
iOS fix needed to build on React Native 0.87's prebuilt core.

- test-app/: react-native-test-app host driven by deep links
  (rnvtest://scenario/<name>); player events rendered as text markers
  with stable testIDs. Two-line RNTA 5.4.9 patch (CLEAR_TOP | SINGLE_TOP
  on the deep-link redirect), see test-app/patches/README.md.
- e2e/: 10 Maestro smoke flows, shared launch/open/wait-for-end subflows,
  local media fixtures served by a dependency-free server with Range
  support, per-RN-version overlays and lockfiles (0.82, 0.87; 0.77 is
  the floor).
- CI: unit.yml (lint, typecheck, unit tests on React 18; library tests
  and typecheck on React 19), e2e.yml PR gate (Android 0.77/0.82/0.87 on
  API 36, iOS 0.87 on iOS 26), e2e-nightly.yml (15-leg grid plus
  quarantine, one issue per failing row, history on the e2e-results
  branch), weekly lockfile refresh, daily cache cleanup, actionlint.
  Third-party actions pinned to SHAs; Maestro and actionlint installed
  from checksummed release archives. No check is required yet.
- Library: ReactNativeVideo.podspec no longer publishes
  Video-Bridging-Header.h (file removed); three React imports are
  framework-style. First unit tests (VideoError, useManagedInstance),
  type-checked and linted through tsconfig.test.json.
- Docs: CONTRIBUTING "Testing your change" and CI sections, PR template,
  e2e/CONTEXT.md (D1-D7), e2e/CI_MATRIX_DESIGN.md (D8-D13, one-time setup).

Stacked follow-ups: #5097 (core fixes), #5098 (Expo config plugin fixes).

Fixes #5084
Closes #5085

Co-authored-by: GratwickEnt <178044584+GratwickEnt@users.noreply.github.com>
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
createSourceFromVideoConfig wrote its defaults back into the object it was given:
initializeOnCreation: true, the platform DRM type and the normalized
externalSubtitles. useVideoPlayer keys the player on JSON.stringify(source), so
a config the caller keeps around (a module constant, a useMemo/useState value)
produced a different key on the next render, and useManagedInstance destroyed
and recreated the player. Build the native config as a new object instead.

The recreate path is inferred from useVideoPlayer/useManagedInstance, not
reproduced in a rendered component; the mutation itself is covered by a test.
sourceFactory: string/asset/config/VideoPlayerSource inputs, the
initializeOnCreation, DRM-type and subtitle defaults, every typed error code,
native errors parsed through tryParseNativeVideoError, and a guard that the
caller's config is not mutated. react-native and nitro-modules are mocked with
mock.module so no native code is involved.
- triggerJSEvent returned true for an event whose last listener had been
  removed (the Set still existed, empty), so throwError treated the error as
  handled and swallowed it: after unsubscribing from onError, play()/pause()/
  seek failures vanished silently. Report false when no listener is registered.
- wrapPromise re-threw from inside its own .catch when nobody listened to
  onError, so the outer promise never settled: await initialize() on a native
  rejection hung forever and logged an unhandled rejection. With a listener it
  rejected with undefined. It now always rejects with the parsed
  VideoRuntimeError and notifies onError listeners first.
- release() called twice inside the 5 s grace window released the native player
  twice; guard with a released flag.
events: every ALL_PLAYER_EVENTS entry routes to the emitter method of the same
name (a new event missing from the switch fails here instead of in an app),
onError stays JS-only, subscriptions remove, duplicates are deduped,
clearAllEvents clears both sides, and trigger reports listener presence.
VideoPlayer: sync methods throw or deliver to onError, unparsable errors are
rethrown, native promise rejections reject with the parsed error whether or not
onError is subscribed, and release is idempotent.

The react-native / nitro fakes move to __tests__/helpers/nativeMocks.ts:
bun's mock.module is process-wide, so two files registering their own fakes for
the same module fought over which one the library saw.
Renders the hook with react-test-renderer. The shared native mocks now record
every player the fake factory creates and keep emitter listeners, so tests can
emit native events and check what was subscribed.
@mziolkowskii
mziolkowskii force-pushed the fix/core-player-errors-and-source branch from 9a931b3 to 3eaa357 Compare September 17, 2026 09:26
@mziolkowskii
mziolkowskii changed the base branch from feat/e2e-infra to master September 17, 2026 09:26
mziolkowskii and others added 7 commits September 17, 2026 11:34
master now type-checks and lints __tests__ (#5096), which these tests predate.
Extend the bun:test typings with mock.module, resolves, toBeDefined,
toBeGreaterThan/toBeLessThan and toThrow(ErrorClass), use the real DrmParams
field (licenseUrl) in fixtures, drop two unused @ts-expect-error directives
and apply Prettier.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
#5135 made onError subscribe once to the native emitter's onError so that
asynchronous load and playback failures reach JS. The routing test still
asserted that onError never touched the emitter and failed after merging
master. It now expects exactly one addOnErrorListener call for any number
of JS listeners; videoPlayerEvents.test.ts covers delivery through it.

Also drop the `() => void x++` shorthand the tests used for one-liners,
which tripped the no-void rule.
…Error

initialize(), preload() and replaceSourceAsync() now always reject with the
parsed VideoRuntimeError; with an onError listener attached they deliver it
there too. The docs and the agent skill said errors are delivered to onError
instead of thrown, which only holds for the synchronous methods.
…onfig

Both branches of the expression are assignable to NativeVideoConfig['drm'];
the cast only hid future type errors.
…cancellations out of onError

wrapPromise re-implemented throwError's parse-then-notify logic, and
replaceSourceAsync() built its source outside the wrapper, so an invalid
source rejected the promise without reaching onError while a native
rejection did both. Both paths now go through reportError, and the async
methods run their native call inside the wrapper so that a synchronous
throw (invalid source, released player) is reported the same way.

A load superseded by a newer initialize()/replaceSourceAsync() rejects
with player/cancelled (source/cancelled on the source side). That is not
a failure, so it no longer reaches onError; the promise still rejects so
the caller can tell. The two codes are added to the error unions, which
did not list them although both platforms emit them.

Tests cover cancellation, the synchronous-throw path of
replaceSourceAsync and its native rejection; the release test is renamed
to what it asserts (the native player is released once), since the
player getter keeps serving the native object during the grace window.
@mziolkowskii
mziolkowskii marked this pull request as ready for review October 7, 2026 10:06
mziolkowskii added a commit that referenced this pull request Oct 7, 2026
…case as root

Registers Expo's own permissions mod after the plugin, as prebuild does, and
runs the manifest mod chain: the permissions must reach the manifest although
Expo's mod runs first. The read-only Podfile case is skipped under root, which
ignores file permissions. The CONTRIBUTING.md sentence is left to #5097, which
rewrites the same line; keeping both edits would conflict at merge.
player/cancelled is not only "a newer load replaced this one": both
platforms also reject every load with it once the native player has been
released, e.g. after replaceSourceAsync(null). Filtering every cancellation
hid that case from onError and left the player silently empty.

Loads started from JS now carry a generation counter. A cancellation whose
load has been superseded by a newer JS load only rejects; a cancellation
with no newer load (a released player) is reported through onError like any
other failure. Covered by two tests, one per case.

Docs and the skill note that the throw/reject rules describe the iOS and
Android players; web has its own VideoPlayer and reports through onError
with web/* codes.
@mziolkowskii
mziolkowskii merged commit 3e4fa7b into master Oct 8, 2026
10 checks passed
mziolkowskii added a commit that referenced this pull request Oct 8, 2026
…vice> (#5098)

* fix(expo-plugins): add the playback service to manifests without <service>

withAndroidNotificationControls pushed onto mainApplication.service with
optional chaining, so a manifest that had no <service> element yet (the default
Expo template) silently got nothing: no VideoPlaybackService, and notification
controls did not work. Create the array, skip the service if it is already
declared, and make sure the permissions array exists before pushing.

writeToPodfile: mergeContents throws when its anchor is missing instead of
reporting didMerge: false, so the warn branch was unreachable and prebuild
aborted on a Podfile without `platform :ios`. Catch and warn as intended.

* test(expo-plugins): cover the config plugins against parsed native files

Runs each registered mod the way prebuild does, with a parsed Info.plist,
AndroidManifest, gradle.properties or Podfile as modResults: background audio
on/off, Picture-in-Picture on .MainActivity, ExoPlayer extension flags with
defaults and stale-entry replacement, the playback service and permissions
(including a manifest with no <service> element and a repeated run),
writeToPodfile anchors for Expo and react-native-test-app, and the plugin
composition per prop.

* docs: list the Expo config plugin tests in the contributing guide

* test(expo-plugins): make the plugin tests pass tsc and ESLint

master now type-checks and lints __tests__ (#5096), which these tests predate.
Extend the bun:test typings with mock.module, resolves, toBeDefined,
toBeGreaterThan/toBeLessThan and toThrow(ErrorClass), and apply Prettier.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* fix(expo-plugins): write the foreground service permissions into the manifest

The plugin added FOREGROUND_SERVICE and FOREGROUND_SERVICE_MEDIA_PLAYBACK to
config.android.permissions from inside its AndroidManifest mod. expo prebuild
applies the app's plugins before it registers its built-in ones, and mods run
starting from the last registered, so Expo's own permissions mod has already
written the manifest when this mod changes the config: the permissions never
reached AndroidManifest.xml. Now that the service is added to the default
template, an app using showNotificationControls or playInBackground would get
the service without the permissions it needs to run in the foreground
(Android 9+, and the media playback type on Android 14+).

The permissions are now added to the manifest itself with
AndroidConfig.Permissions.ensurePermissions, which does not add them twice,
and the tests assert the manifest rather than the config.

Verified by compiling the manifest mods with @expo/config-plugins 10.1.2 on
Expo's bare template, with this plugin registered before and after the
permissions mod: the service and both permissions are present exactly once.

* fix(expo-plugins): add permissions via withPermissions, keep extension defaults

Review follow-ups on this PR:

- withAndroidNotificationControls: use AndroidConfig.Permissions.withPermissions
  at plugin level instead of ensurePermissions inside the manifest mod. It adds
  the permissions to config.android.permissions when the plugin is applied, so
  Expo's own permissions mod writes them whenever it runs, registers a manifest
  mod of its own, and keeps `expo config` truthful; this is also what v6 did.
  The warning for a manifest without a main application named the wrong
  element (.MainActivity, copied from the PiP plugin).
- withAndroidExtensions: a key left out of a partial object now keeps its
  documented default (true) instead of switching the extension off, so
  { useExoplayerHls: false } no longer disables DASH as well.
- writeToPodfile: only a missing anchor (ERR_NO_MATCH) is downgraded to a
  one-line warning naming the anchor; any other error (an unwritable Podfile)
  still aborts prebuild.
- Tests: the config type comes from ConfigPlugin instead of a direct
  @expo/config-types import the package does not depend on; the test-app
  Podfile case guards against indexOf returning -1, which made it pass
  vacuously; new case for a Podfile that cannot be written.

* docs(expo): list what the config plugin changes on each platform

The page did not say that the plugin always registers the Android playback
service and its two foreground permissions, nor what each option writes.
enableBackgroundAudio was described as an Android feature; it only edits
UIBackgroundModes in Info.plist, Android uses the playback service instead.

* test(expo-plugins): cover the prebuild mod order, skip the read-only case as root

Registers Expo's own permissions mod after the plugin, as prebuild does, and
runs the manifest mod chain: the permissions must reach the manifest although
Expo's mod runs first. The read-only Podfile case is skipped under root, which
ignores file permissions. The CONTRIBUTING.md sentence is left to #5097, which
rewrites the same line; keeping both edits would conflict at merge.

* feat(expo-plugins): add enableAndroidPlaybackService to opt out of the playback service

The plugin has registered VideoPlaybackService and the FOREGROUND_SERVICE /
FOREGROUND_SERVICE_MEDIA_PLAYBACK permissions for every app since #4754 (it
only looked otherwise because neither reached the manifest; fixed earlier in
this PR). That stays the default: the player starts the service at runtime
when playInBackground or showNotificationControls is set, which prebuild
cannot know. An app that uses neither can now pass
enableAndroidPlaybackService: false to keep the service and permissions out
of its manifest, e.g. to avoid the foreground service declaration in the
Play Console. Documented on the Expo page and in the agent skill, where the
Android background note wrongly pointed at enableBackgroundAudio (iOS-only).

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Bart Widlarz <bartlomiejwidlarz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants