Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(inIframe/isEmbedded) add ability to detect generic embedding #15724

Merged
merged 2 commits into from
Mar 26, 2025

Conversation

saghul
Copy link
Member

@saghul saghul commented Mar 7, 2025

On web we detect if we run on an iframe, and on mobile we detect if the app is one of ours.

@saghul saghul force-pushed the is-embedded branch 3 times, most recently from 3a0e93f to 2696d64 Compare March 7, 2025 13:38
@damencho
Copy link
Member

damencho commented Mar 7, 2025

LGTM, but let's wait and for @hristoterezov :)

Copy link
Member

@hristoterezov hristoterezov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still like the alternative approach better where we allow to override the options for all RN use cases and then have a whitelist in the JitsiMeet App for the things we care about from the apps perspective. In practice this is the same thing and we just have exception for the JitsiMeet App but for any other RN app that uses the SDK those will be allowed and it will be apps responsibility to not pass unverified values for these props. So I don't see much value in the exception for our app...

But this is personal preference and I'd say even this approach is acceptable although I like more the other one.

Added some questions and comments. I think we can definitely reduce the list of the allowed config props.

@saghul
Copy link
Member Author

saghul commented Mar 25, 2025

@hristoterezov PTAL. It now includes the timeout implementation for mobile.

@saghul
Copy link
Member Author

saghul commented Mar 25, 2025

Jenkins please test this please.

* Additional interface config whitelist extending the original whitelist applied when Jitsi Meet is embedded
* in another app be that with an iframe or a mobile SDK.
*/
export default [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this array needs to be empty!

saghul added 2 commits March 26, 2025 23:13
On web we detect if we run on an iframe, and on mobile we detect if the
app is one of ours.
@saghul saghul merged commit 0a467f7 into jitsi:master Mar 26, 2025
16 of 18 checks passed
@saghul
Copy link
Member Author

saghul commented Mar 26, 2025

@damencho Tests passed but the CI choked on something regarding my branch:

Switched to a new branch 'is-embedded'
+ git pull https://github.com/saghul/jitsi-meet.git is-embedded
From https://github.com/saghul/jitsi-meet
 * branch                is-embedded -> FETCH_HEAD
fatal: It seems that there is already a rebase-merge directory, and
I wonder if you are in the middle of another rebase.  If that is the
case, please try
	git rebase (--continue | --abort | --skip)
If that is not the case, please
	rm -fr ".git/rebase-merge"
and run me again.  I am stopping in case you still have something
valuable there.

Build step 'Execute shell' marked build as failure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants