Improve script injection for native bridge

[Maxr1998]

I'm for some reason not really happy with this yet.. feedback is highly appreciated 😇

[nielsvanvelzen]

Review notes:

• Some users don't have web in the server but do have it on the same domain, so "/web/index.html" does not exist
• The redirect path might end up being "/" this might cause a lot of bad interceptions
• The way of detecting the redirect feels ugly

Some suggestions on other ways this issue could be fixed:

• Add a second field when connecting to a server called "Web UI", which is hidden by default under "advanced options". When it's empty the server url will be used by default (the value of the server field could perhaps be used as placeholder in de webui field)
• 1. Intercept all HTML requests and detect (based on the source) if it's the Jellyfin client's index. If so: add injection
  2. Or intercept the "scripts/apploader.js" file and add some additional JavaScript to load our injections

I think the apploader.js approach might be one of the best solutions but we need to make sure the web team doesn't change it without warning in an update.

[thornbill]

I think the apploader.js approach might be one of the best solutions but we need to make sure the web team doesn't change it without warning in an update.

Good luck with this one... Browser profile changes in web have broken things in the iOS app a few times lol

I’ve been thinking about this also and I was wondering about having the config.json file point to the server. Then a user could enter the web url which probably makes more sense to them. Then we can grab the config file to get the server url.

Not sure if that’s the best approach or not. It’s something that should probably be discussed with web and all the app teams that use web though to agree on a single approach.

[Maxr1998]

• Some users don't have web in the server but do have it on the same domain, so "/web/index.html" does not exist

This was the issue I was trying to work around - I still kept it as a fallback cause it works on most setups, I think.

• The redirect path might end up being "/" this might cause a lot of bad interceptions

This would be very unfortunate - I hadn't thought of that, but it may certainly be possible.

• The way of detecting the redirect feels ugly

I didn't say I was happy with my code 🤣

Some suggestions on other ways this issue could be fixed:

• Add a second field when connecting to a server called "Web UI", which is hidden by default under "advanced options". When it's empty the server url will be used by default (the value of the server field could perhaps be used as placeholder in de webui field)

Certainly possible, but isn't the most user-friendly option. I'd prefer to avoid that.

• 1. Intercept all HTML requests and detect (based on the source) if it's the Jellyfin client's index. If so: add injection

Unfortunately, WebView doesn't really allow this :/
The resource override function only supplies the request path, not the path it loaded the file from, ignoring redirects. Also, reverse-proxies can still break this, if they resolve the redirect internally.

2. Or intercept the "scripts/apploader.js" file and add some additional JavaScript to load our injections

I think the apploader.js approach might be one of the best solutions but we need to make sure the web team doesn't change it without warning in an update.

Good luck with this one... Browser profile changes in web have broken things in the iOS app a few times lol

JS files would definitely be easier to hook, but injecting stuff on a minified JavaScript file.. is a recipe for disaster xD

What I had thought of would be always requesting an empty script in the webapp index, like scripts/native.js, which throws a 404 on webservers, but can easily be hooked and completely replaced in apps. That'd require changes to the webapp though.

It’s something that should probably be discussed with web and all the app teams that use web though to agree on a single approach.

I agree, I'll bring this up in the chat later.

[Maxr1998]

Another try, this solution should be much better. Theoretically, there could be race conditions in the new code because of the dispatch to the UI thread, but I haven't seen any signs of one happening yet, so it should be fine.

[Maxr1998]

Loading the about:blank doesn't seem to fully work, but I'd still keep it in for now. It's not such a big issue anyways.

[nielsvanvelzen]

Suggested change

	const val JS_INJECTION_CODE = """
	!function() {
	var scripts = [
	'/native/nativeshell.js',
	'/native/apphost.js',
	'/native/EventEmitter.js',
	'/native/chrome.cast.js',
	];
	scripts.forEach(function(src) {
	var scriptElement = document.createElement("script");
	scriptElement.type = "text/javascript";
	scriptElement.src = src;
	scriptElement.charset = "utf-8";
	scriptElement.setAttribute("defer", "");
	document.body.appendChild(scriptElement);
	});
	}();
	"""
	const val JS_INJECTION_CODE = """
	!function() {
	var scripts = [
	'/native/nativeshell.js',
	'/native/apphost.js',
	'/native/EventEmitter.js',
	'/native/chrome.cast.js',
	];
	scripts.forEach(function(src) {
	var scriptElement = document.createElement('script');
	scriptElement.type = 'text/javascript';
	scriptElement.src = src;
	scriptElement.charset = 'utf-8';
	scriptElement.setAttribute('defer', '');
	document.body.appendChild(scriptElement);
	});
	}();
	"""

[nielsvanvelzen]

I still prefer a separate file for this but I guess that won't happen 😂

[Maxr1998]

I think the overhead just isn't worth it.. I wanted to add a @Language annotation to inject a language reference, but Android Studio (or IntelliJ CE) doesn't support JavaScript 😤