Shopify / shopify-app-bridge

https://shopify.dev/docs/api/app-bridge
85 stars 9 forks source link

App is incorrectly reported as outdated if it takes too long to load #160

Closed matthewhilton closed 3 months ago

matthewhilton commented 1 year ago

Describe the bug

If an app takes too long to load, it will be incorrectly reported as being outdated when using an admin.shopify.com domain (aka unified admin)

I have checked my app configuration (app bridge, frame ancestors headers, etc...) and all is following the guidelines.

The error originally showed occasionally when the app loaded for the first time in a new browser, which makes sense as the load time will be slightly longer due to caching js, css, etc. This would happen about 1 in 10 loads.

If I introduce an artificial sleep of for e.g. 5 seconds in my app loading route, the error shows up 100% the time.

image

To Reproduce

Steps to reproduce the behaviour:

Prerequisites:

  1. Be on the new unified admin domain e.g. admin.shopify.com/stores/xyz
  2. Introduce an artificial sleep of 5 seconds in the app load script to simulate a slow network connection / fresh load of app

Reproduce:

  1. Launch app
  2. Error is shown

Expected behaviour

I am guessing it is checking for the existence of the AppBridgeProvider, albeit slightly too early. However since App bridge is closed source this is just a guess.

I would expect that the Admin UI waits until the app is completely loaded before checking for its existence.

Contextual information

Packages and versions

Platform

mbaumbach commented 1 year ago

This message is confusing merchants and partners alike. Partners aren't sure how to fix this (or if it's a bug) and merchants are concerned that apps are no longer going to function. We've only had a couple merchants bring it up, but we can't reproduce it without introducing the 5 second timeout mentioned in this issues' description.

I feel like this message should be presented to partners in the partner portal if there's an issue (similar to the API deprecations) and it should be documented what needs to be addressed to no longer be outdated. Presenting this to merchants only, especially with deprecation about a year out, seems like it is causing unnecessary concern for everybody.

If this isn't a bug, I imagine the five second limit is also a bit tight for some partners. Especially on a first load with uncached resources. It’s also greatly dependent on the network connection of the merchant. If a single merchant crosses this threshold does that mean the app is no longer up to date and could be removed for all merchants? Is there a more deterministic measure of app performance that could be implemented here?

aw-katusev commented 1 year ago

This is definitely something merchants shouldn't see. But partners need to have it in the Apps -> app details page so they know what to do. Shopify usually sends notifications in partenr panel, via email or marks as a mandatory api update. This case is not an exception.

Also, we successfully passed app audit and introduced necessary fixes where it was necessary less than two months ago. So we were confident that everything is fine from technical perspective.

matthewhilton commented 1 year ago

I should make it clear the 5 seconds example I gave is arbitrary - I do not know the actual time it is testing or if it is even checking after X number of seconds at all. It could actually just be a coincidence that this check happens after loading xyz script that just so happens to take X seconds to load.

I am using serverless functions to load my app, and so this happens every time there is a cold start which means usually ~ 5-6 seconds but after that every load is < 2 seconds and so does not display this message.

While I can workaround the issue by hyper optimising and pre-warming my serverless functions - the root cause of this problem is that this check should actually just wait for the script to actually return some HTML or wait for the DOM to settle.

There's also the problem that as partners we cannot create test stores using the new unified admin admin.shopify.com - the only reason I discovered this bug was because the app reviewer (who do have access to the unified admin) ran into the bug and allowed me collaborator access.

matthewhilton commented 1 year ago

I have done the following to try and debug this issue:

  1. Test pre-warming serverless functions. - This did not work, I have watched a 500ms load (according to the timings tab in FF) of the app/load function be shown this error
  2. Test a smaller bundle size. My app loads a serverless function before redirecting to the HTML / React JS bundle. Since my App uses NextJS and has a non trivially sized JS bundle, I was thinking maybe the downloading of the bundle was too slow. So I tried making it return instead a plain HTML page with only the minimum JS to initiate an app bridge connection. - This did not work either.

The only way I can consistently reproduce it is to force an unnecessarily large loading time (e.g. 5 seconds), however, this should theoretically mean that fast loads (e.g. 500ms) shouldn't be shown this error, but this is not the case.

Without the artificial app slowdown, I have noticed the issue only appears once after not using for about an hour and is only shown once. After that is it not shown again. Clearing browser caches, using private nav, etc... does not cause this issue to resurface.

This leads me to believe it could be a combination of my apps loading time + the shopify admin loading time. I will keep doing some more tests.

AaronSpringut commented 1 year ago

Hey folks. This alert is part of the warning for our upcoming changes to the url structure. At the moment, only merchants in the beta programs for these changes will see the banner. Part of the current detection for apps that do not follow the guidance is a timeout period. If an app does not initialize an up to date version of App Bridge before the timeout, the Shopify admin assumes the app is not compatible with the upcoming changes and a warning is shown to direct the user to the myshopify domain.

In the short term, we’re going to increase the timeout for some extra margin of safety, however, it’s already quite a long period for a user to wait for an app load. A bit longer term, we’re working on an update to this logic that does not rely on this timer, and looks at the app’s past behavior to determine if an app is compatible with the upcoming changes, and whether to show the banner.

mbaumbach commented 1 year ago

In the short term, we’re going to increase the timeout for some extra margin of safety, however, it’s already quite a long period for a user to wait for an app load. A bit longer term, we’re working on an update to this logic that does not rely on this timer, and looks at the app’s past behavior to determine if an app is compatible with the upcoming changes, and whether to show the banner.

Thanks for the update @AaronSpringut, that change sounds encouraging!

itsnotworkingatall commented 1 year ago

@AaronSpringut here are some thoughts and questions:

  1. We were able to reproduce the problem using "Slow 3G" network setting in Chrome. So a simple condition of "N seconds loading" is too vague. At least let's use clear benchmark like "App should be loaded in X seconds with a "Slow 3G" network setting in Chrome". And this should be published in your requirements along with content security policy, app bridge version, and the host parameter.
  2. The text of the alert is way too misleading. Can you at least change it to make more sense both for merchants and app developers?
  3. Again, why do you show it to the merchants in the first place? Its 10 months before the deadline. By that time we could literally rewrite a whole app from scratch if needed. Thanks.
cyberabis commented 1 year ago

Hey folks. This alert is part of the warning for our upcoming changes to the url structure. At the moment, only merchants in the beta programs for these changes will see the banner. Part of the current detection for apps that do not follow the guidance is a timeout period. If an app does not initialize an up to date version of App Bridge before the timeout, the Shopify admin assumes the app is not compatible with the upcoming changes and a warning is shown to direct the user to the myshopify domain.

In the short term, we’re going to increase the timeout for some extra margin of safety, however, it’s already quite a long period for a user to wait for an app load. A bit longer term, we’re working on an update to this logic that does not rely on this timer, and looks at the app’s past behavior to determine if an app is compatible with the upcoming changes, and whether to show the banner.

Hi @AaronSpringut , would like to bring a scenario to your notice. With multiple apps we are seeing the alert when the user signs up for the first time (when there is no user session in the Redis). The alert does not appear on subsequent app visits. So, I am guessing the very first app access with no session takes more time and hence the alert is showing up. It would be a very bad experience for a newly signing up customer to see this alert. We have made all other changes (App bridge 3, CSP Header, Host name etc.) and tested this even with light weight apps. Kind of in a dead-end on solving this. I'll be happy to share more details of our app if you need.

traianturcu commented 1 year ago

Is there a way to migrate/force one of our dev stores to use the new URL (admin.shopify.com/store/..) so we can test this specific issue for our apps?

cyberabis commented 1 year ago

You can reach out to Partner support and raise this request. I think they call it as "Beta" flag for the new admin domain.

juona commented 1 year ago

@AaronSpringut

If an app does not initialize an up to date version of App Bridge before the timeout, the Shopify admin assumes the app is not compatible with the upcoming changes and a warning is shown to direct the user to the myshopify domain.

Well, what if my app does not use App Bridge? :/ I still expect it to be launched from within Shopify via an iframe. That's what the app reviewer is attempting to do as well, as part of the review process. Unfortunately, it fails to load when launched from within this new unified admin, even though I have the correct CSP headers set (same error as the OP). Is this an oversight? Or are all new apps expected to use App Bridge?

sandeepgit14 commented 1 year ago

@AaronSpringut we are facing the same popup always on one Shopify store and as per the Shopify guidelines, the app bridge is updated to the latest and the host parameter is also working. The Content-Security-Policy headers are also verified and there is no issue.

The app build is also upgraded and it's loading in less than 1 second but still, the issue is the same so it's not an issue with a timeout. I am not able to find any specific reason for the above popup issue.

The same code is working fine on the staging app and there is no such issue.

Any suggestions are most welcome!

AaronSpringut commented 1 year ago

@juona

@AaronSpringut

If an app does not initialize an up to date version of App Bridge before the timeout, the Shopify admin assumes the app is not compatible with the upcoming changes and a warning is shown to direct the user to the myshopify domain.

Well, what if my app does not use App Bridge? :/ I still expect it to be launched from within Shopify via an iframe. That's what the app reviewer is attempting to do as well, as part of the review process. Unfortunately, it fails to load when launched from within this new unified admin, even though I have the correct CSP headers set (same error as the OP). Is this an oversight? Or are all new apps expected to use App Bridge?

Using App Bridge is a requirement for App Store apps that are embedded in the Shopify Admin.

AaronSpringut commented 1 year ago

@sandeepgit14

@AaronSpringut we are facing the same popup always on one Shopify store and as per the Shopify guidelines, the app bridge is updated to the latest and the host parameter is also working. The Content-Security-Policy headers are also verified and there is no issue.

The app build is also upgraded and it's loading in less than 1 second but still, the issue is the same so it's not an issue with a timeout. I am not able to find any specific reason for the above popup issue.

The same code is working fine on the staging app and there is no such issue.

Any suggestions are most welcome!

Can you share a link to your app? If the app is using AB 2/3, uses the host parameter, and has the CSP set correctly it should work. To clarify, the page has to initialize the App Bridge library within the current timeout period. If the page loads but App Bridge is not initialized, the Admin does not see the necessary message to prevent the banner from being displayed.

Keemtaker commented 1 year ago

Is there a temporary fix to this @AaronSpringut @matthewhilton ? I am currently stuck on the app review process because of this issue

lukasz-pw commented 1 year ago

@AaronSpringut I agree with one of the comments that the popup message is misleading. It can affect trust in our app even though it may be a problem with the client's network speed, and not our app specifically. I think it's fair to make a loadtime a requirement for an app, and should be monitored and communicated to app owner, but it's not entirely fair to show a popup suggesting that the app is outdated and will no longer function.

I realize that this affects only users using the new admin access URL, but this should be escalated as it may affect our business.

psppro26 commented 1 year ago

@AaronSpringut I'm totally agree with others. My app is totally is up to date with the last requirements and i got some merchants reaching me ( even some asking for a full refund ) cause my app will be "outdated" and is no longer maintened. I dont see the point to display this message to merchants ? why dont just send the alerts trough partner dashboard like API ones ? Of course it's only affect users using the new admin access URL. Thanks

matthewhilton commented 1 year ago

@Keemtaker The only way to really reduce this is to make your app load as fast as possible. I am using NextJs so it takes a while to download and cache the JS bundle, so what I did to mitigate this was to load a very simple HTML page with a JS script that just loads the app bridge, which would then redirect to the main app. I have since removed this though, as I think the increased timeout that was implemented has "fixed" this issue.

I highly suspect it is also at least partially dependent on the speed at which the shopify admin itself loads - which is out of your control.

Also just an FYI, if you email partner support they change a flag on the backend so that one of your dev stores can use the new unified admin domain so you can test this yourself.

derrickrc commented 1 year ago

In my testing, the workaround to this is to initialize createApp with the host parameter as soon as possible on the client side (even before authenticating the user or the rest of your app is loaded, if needed) - you have about 5-6 seconds to do so after the user initially arrives at your app. This should prevent the message from showing, and whether or not you have CSP headers doesn't affect this message coming up based on my testing.

assaflei commented 1 year ago

Hi, The recommendations provided here reference the app bridge javascript API Is there a best practice recommendation for using the react component of app bridge (details)? My app bridge is initialized as part of the app render (the config object contains the host parameter)

<Provider config={config}>
    <AppProvider i18n={translations}>
        <Component {...pageProps} />
    </AppProvider>
</Provider>

I don't see how this can be separated from the rest of the app since its the app itself (I use nextjs to bundle everything)

mingfengwan commented 1 year ago

@AaronSpringut My app is literally built from a Shopify app template with every single library up to date, and this message still shows up. Developers should at least expect the app template provided by Shopify to be "error-free" instead of wrapping their heads around this.

derrickrc commented 1 year ago

Hi, The recommendations provided here reference the app bridge javascript API Is there a best practice recommendation for using the react component of app bridge (details)? My app bridge is initialized as part of the app render (the config object contains the host parameter)

<Provider config={config}>
    <AppProvider i18n={translations}>
        <Component {...pageProps} />
    </AppProvider>
</Provider>

I don't see how this can be separated from the rest of the app since its the app itself (I use nextjs to bundle everything)

@assaflei I haven't used App Bridge react, but like you said it looks like the Provider config object is equivalent to the JS createApp method. I think as long as you return Provider within the first 5 seconds of the request, it should prevent the message from showing.

assaflei commented 1 year ago

Hi, The recommendations provided here reference the app bridge javascript API Is there a best practice recommendation for using the react component of app bridge (details)? My app bridge is initialized as part of the app render (the config object contains the host parameter)

<Provider config={config}>
    <AppProvider i18n={translations}>
        <Component {...pageProps} />
    </AppProvider>
</Provider>

I don't see how this can be separated from the rest of the app since its the app itself (I use nextjs to bundle everything)

@assaflei I haven't used App Bridge react, but like you said it looks like the Provider config object is equivalent to the JS createApp method. I think as long as you return Provider within the first 5 seconds of the request, it should prevent the message from showing.

@derrickrc you suggest that I add a short js file executing the create app and follow on it with the rest of the nextjs app?

derrickrc commented 1 year ago

@assaflei yes that would be my suggestion, but in my testing if you try to initialize createApp when a merchant does not yet have your app installed, Shopify will do a hard redirect to show an error page that they don't have your app installed and it could break your OAuth flow.

I didn't get around to trying to wrap it in try / catch which might mitigate the above, but otherwise only initialize createApp after you've determined on your end that the merchant already has your app installed.

ctrlaltdylan commented 1 year ago

My customers reporting seeing this error message as early as Saturday. My customers are thinking I'm shutting down and I'm losing revenue because of this terrible UX choice.

Why are my customers getting this notice?

I should be receiving direct communications from Shopify to warn me about this, the messaging is vague and I have not received any email specifically about this issue from Shopify.

This banner policy needs to be reverted. Please give error messaging to partners not to end customers.

And please communicate exactly what is the issue.

patrickbolle commented 1 year ago

Also seeing this on my app that is fully updated with no API version complaints, host parameter set, app bridge 3.0. App is not slow to load and app bridge initialization is happening as soon as possible. Merchants are getting incredibly confused. This is an awful and confusing message to show to end users.

charles-tyler commented 1 year ago

Also facing the same issue. Very bad visual to Merchants, nothing wrong with our app. Shopify should alert Partners, not the Merchants.

andrewle commented 1 year ago

Facing this issue as well across multiple merchants. We're doing all the right things already regarding API versions, host parameter, and app bridge version as well as page speed. We're afraid merchants are uninstalling our app due to this message

assaflei commented 1 year ago

Testing a workaround for loading a short script with createApp and than proceed with the /auth flow encounters a bug I use the @shopify/koa-shopify-auth package As part of its execution it uses the app bridge's redirect action For using it, it uses the createApp command including shopOrigin property containing the *.myshopify.com host name

var app = createApp({
    apiKey: window.apiKey,
    host: window.host,
    shopOrigin: window.shopOrigin.replace(/^https:\\/\\//, '')
    });

I noticed that using this shopOrigin value causes the navigation to fail, maybe due to dashboard domain change? The shopify-auth package github repo looks deprecated so I'm not sure who can look into this and suggest some change (problematic file there is @shopify\koa-shopify-auth\dist\src\auth\client\storage-access-helper.js)

clement911 commented 1 year ago

Same here. We use app bridge 3.0 and have admin.shopify.com in our CSP. App is fast to load. Merchants are confused. So are we...

forsbergplustwo commented 1 year ago

We're getting reports of this as well on our apps since this weekend. Running on AppBridge 3.0, CSP updated and app is faster than 5 secs to load. Not communicating all this to partners and instead showing a message about something changing in 9 months is kind of the opposite of a professional rollout. It's a major change affecting app developers and their customer's experience, with incorrect text and triggering.

Would be great if this could be looked into with priority please @AaronSpringut 😊

EDIT:

I can consistently reproduce this by enabling "Disable caching" and enabling "Slow 3G" in network tab of Chrome, while running any installed embedded app on a admin.shopify.com enabled store, including Shopify's own apps:

CleanShot 2023-01-19 at 16 03 28

cleverlight commented 1 year ago

@AaronSpringut I'd also like to question the logic that says when an app is non-compliant or too slow, we should tell a storeowner that it needs updating: a storeowner who has no possible way to update it. It's not only disempowering for them, it's scary because they feel like something bad is about to happen (albeit in 9 months) and the only person who can do something about it is someone they don't know!

Perhaps instead we could have an "app test" page in the Partner dashboard, that applies the current set of requirements to the app, measures its performance and highlights any shortfalls directly to the partner, who can then do something about it.

AaronSpringut commented 1 year ago

Thanks again for bringing this to our attention. To support Shopify admin’s migration to a new unified domain—admin.shopify.com—embedded apps were required to make updates, such as moving to App Bridge 3.0, to ensure the best merchant experience. Both the standard version of App Bridge and the React version (using the Provider component) will fulfill this requirement. The best practice is to initialize App Bridge as soon as possible when loading your site.

For apps that have not yet made these required updates, a blocking error message will be presented upon app load. However, this error message was erroneously presented for apps that had already been updated but were loading slowly. We apologize to all partners impacted by this.

We’re working on a resolution that will provide clearer error messaging, and prevent it from being shown erroneously for apps that have made the required updates. You will continue to receive updates via email and through the partner dashboard.

Note: To ensure your app does not experience issues as the migration continues, if you have not already, please implement the required updates detailed here.

janklimo commented 1 year ago
wut

Seeing this warning as of today for no obvious reasons. I've implemented the requirements as @AaronSpringut mentioned above more than a month ago.

I'm returning my app's HTML file in under 200ms, CSP headers are set. App bridge is loaded immediately. Any pointers?

muraliavarma commented 1 year ago

I have the same exact problem as above commenter, even after upgrading to app bridge v3, adding in the new CSP entry. Interesting thing is that my local version that runs on ngrok works fine while my production build is throwing this error. Totally confused about this.

kragotte commented 1 year ago

Hi all,

This banner should now be checking to see if your app has ever loaded successfully in the new admin domain (admin.shopify.com). We will not redirect the merchant or show the banner if the app ever has loaded successfully. Testing apps through partner dev stores (all of which should now be on the new admin domain) is the fastest path to us recording a successful load.

For apps that have been recently updated or never yet loaded in admin.shopify.com, the banner will temporarily still show. Our data pipeline is updated approximately every 3hrs. In the meantime, confirm that your app is loading correctly on admin.shopify.com by ensuring that sure your app is loading beneath the banner displayed on admin.shopify.com.

Since your comments were about 5-6hrs ago, you should not be seeing the banner any more as long as your app loaded successfully during your testing. If you are seeing your app load on admin.shopify.com, but are still seeing the banner and getting redirected, please reach out to partner support (or share a link to your app here) so that we can dig into your specific situation further.

janklimo commented 1 year ago

Confirmed, the warning is gone now that I checked it 9 hours later 👍

muraliavarma commented 1 year ago

This partially works. The warning went away for one of the stores that had the app installed. But new stores that are installing this app is still showing the banner

kragotte commented 1 year ago

@muraliavarma I sent you an email requesting more details about your app. We'll need to investigate why you're still seeing the redirect on new stores.

avtans-kumar-shipsy commented 1 year ago

@kragotte @AaronSpringut I have created a new app which uses App Bridge 3 ("@shopify/app-bridge": "^3.1.0") out of the box, for which it was mentioned that the apps created by Shopify CLI 3.x is already setup to handle the unified admin domain changes (https://shopify.dev/apps/tools/app-bridge/getting-started/app-setup). But even after that, on successful installs of my app on any development test store, 2 tabs open up - one with admin.shopify.com and the other with storename.myshopify.com with force_legacy_domain=1 as query params.

The banners showed up initially and as per the response, it seems that the banner is no longer showing up, but I'm still getting redirected. Any idea what is going wrong here?

dani-sanomads commented 1 year ago

I'm facing the same issue mentioned by @avtans-kumar-shipsy . Any Updates? @kragotte @AaronSpringut

Mark-H commented 1 year ago

While the banner is gone, it's still opening the extra tab on an app that was working fine a week ago (and still works fine in both tabs). I've reached out to partner support as well (36184225) and will edit when I hear back, but has anyone found a way to reset this behavior? It's also coming up a lot in the developer discord.

rosantoz commented 1 year ago

I've been affected by this issue as well. Everything is running on the latest version. Banner shows up. A new tab opens.

Even a fresh app created with cli 3.42.0 has the same issue.

After so many people reporting the same, it's time for the dev team to acknowledge the problem and give us a solution.

kragotte commented 1 year ago

Hi all - apologies for the delayed response, we have identified a bug related to showing the banner on new installs of recently updated apps. We are actively working on a solution. Please note that while all partner dev stores are on the new domain URL, the vast majority of merchants are not.

While we work on the bug fix, we do have a manual process in place to opt out apps that have shown a successful load in the new admin domain. If you retry your apps this morning, if they have successful loaded successfully in the new admin domain, they should no longer be showing the banner or redirect on existing installs or new installs. If you are still experiencing issues please @ me.

mithatb commented 1 year ago

Hi all - apologies for the delayed response, we have identified a bug related to showing the banner on new installs of recently updated apps. We are actively working on a solution. Please note that while all partner dev stores are on the new domain URL, the vast majority of merchants are not.

While we work on the bug fix, we do have a manual process in place to opt out apps that have shown a successful load in the new admin domain. If you retry your apps this morning, if they have successful loaded successfully in the new admin domain, they should no longer be showing the banner or redirect on existing installs or new installs. If you are still experiencing issues please @ me.

Hi all - apologies for the delayed response, we have identified a bug related to showing the banner on new installs of recently updated apps. We are actively working on a solution. Please note that while all partner dev stores are on the new domain URL, the vast majority of merchants are not.

While we work on the bug fix, we do have a manual process in place to opt out apps that have shown a successful load in the new admin domain. If you retry your apps this morning, if they have successful loaded successfully in the new admin domain, they should no longer be showing the banner or redirect on existing installs or new installs. If you are still experiencing issues please @ me.

Hi is this related with shopify tries open new tabs for embedded apps? I have been trying to figure out the reason last 3 hours.

qq99 commented 1 year ago

firefox_MxGKarORl7

@kragotte Hi, this is still happening with my app. I noted that it was fine on my first new dev tester store (~qq99-tester-store.myshopify.com~), but I made a 2nd dev tester store (~qunit-price-demo-store.myshopify.com~) today to verify the fresh install process. 2 tabs (one with new subdomain, one with legacy), maybe 3 or 4 redirects going on in both. Both are dev stores w/ the admin. subdomain. Interestingly, the embedded app experience is totally fine on the first store, but is constantly opening a new tab on the 2nd store. The app is Qunit Price

I'm on Starlink, so hopefully this is not related to client-side load times because mine are always terrible 😂

Edit: I've since made those 2 apps defunct, but you should be able to reproduce on https://admin.shopify.com/store/qunit-price-demo-store/apps/unitwise

rosantoz commented 1 year ago

Thanks @kragotte

This is still happening to me. It wouldn't be a problem to wait it was happening to a live app.

However, one of my apps in going through the approval process (app id 5726619) and the app review team won't approve it because of this issue. I've sent them a link to this thread but they insist I need to fix it.

Is there an ETA on when this will be fixed? Is it possible to opt out this particular app so I can get it approved?

Mark-H commented 1 year ago

Just for the record this issue was fixed for my app since Kragotte's message with no action from partner support (or at least, no message from them). So something did get fixed. Do your apps actually respond quickly, and have the right security headers + app bridge 3?

rosantoz commented 1 year ago

@Mark-H In my case yes, they do. However the problem persists for new installs.

@kragotte confirmed there's an issue (in the reply above):

we have identified a bug related to showing the banner on new installs of recently updated apps

avibz1987 commented 1 year ago

any news about this?