I think this could have been done in a much smaller change, something along the lines of:

const defaultedAlt = alt ? alt : sprintf( __( 'This image has an empty alt attribute; its file name is %s' ), this.getFilename( url ) );
const img = <img src={ url } alt={ defaultedAlt } onClick={ this.onImageClick } />;

Would avoid the need for the if statement on line 486 and the ternary on 574.

great suggestion. that makes this much more readable.

It looks like the regular expression doesn't take query string parameters into account, and I think it should strip them.

For example, the image for my gh avatar has some params in the url -> https://avatars2.githubusercontent.com/u/677833?s=60&v=4

I also think the toString might be unnecessary, I'd expect url to be a string or undefined given that's its attribute type.

Thanks! Nice callout. My regex is horrible so I would love a review on my recent commit. I tested with a regular media library image and also the avatar string you provided. Seems to be working! 😬

One change I'd make is to for the second group to be a non-capturing group, since we're not concerned with using its matched content:

/.*\/(.+?)(?:\?.*)?$/

Cool! Good call. Thanks!

This shouldn't be changed. We need to reenable any ESLint immediately after it is no longer necessary to be disabled.

Won't this produce a nonsense alt value of 'This image has an empty alt attribute; its file name is ' if this return is reached?

@aduth Thanks! I do not believe there is an instance that url will not pass given the Media Library is where this is passed back from. I can't imagine how it would not return url. I only added this check as an edge case precaution of a graceful return instead of an error of using an undefined url. Can you think of any edge cases where url would return false? Happy to remove the check; I just want to make sure that I'm not taking away a safety net. :)

Well, ideally we're certain the sort of data we're expecting. For something like URL user input, it's a bit harder to nail down. But maybe it's safe to assume that every URL should at least have a / followed by a "filename" (this is what the pattern is checking, yeah? The fact I'm asking is maybe a hint it's not very obvious on its own). If we are going to be uncertain, we should at least follow-through on our uncertainty, where defaultedAlt doesn't perform the substitution at all if the URL can't be determined.

The main reason I'd see to author this way vs. directly return the value (return url.match( /* ... */ )[ 1 ]) would be to have the variable serve as an explaining variable, but in this case its not well-explained in that the return of String#match is not the file name, it's a match Array.

This could be made more durable (especially the query parameter) bit if we were to rely on the new getPath from @wordpress/url.

const path = getPath( url );
if ( path ) {
	return last( path.split( '/' ) );
}

To our previous conversation about the certainty of the file name being matched; is there any guarantee that the input here is in-fact a url? It's making me a bit nervous.