PWA-Chrome issue #937
Comments
arturv2000
added
bug
|
@arturv2000 @cstns Seems we made error handling too specific… I was afraid this would happen if we tried to add conditions. Can you show a console log of the error type so we can add it to error handling? |
|
@cgjgh Hi, No idea if doing this in the correct way, compiled in 'dev' and was able to put a breakpoint on source code, the error message is the one shown..
Tried in ´npm run build´ and added an extra console.log just to be sure, and booth where printed to the console. |
|
@arturv2000 For additional context, could you insert breakpoints into the code within the @colinl I suspect you might be having the same issue and getting the above error as well. @cstns I realize your previous concern of a reload loop occurring by not specifying the errors that would trigger a reload. However, if the errors are too diverse, specificity might not be practical. Also, the original, pre-PWA error handling, which console logged “auth error”, suggests errors were mostly occurring with authentication- (@joepavitt Can you weigh in on this?). |
|
Yes, I am seeing issues two. I have looked in a bit more detail at what is going on, with the dashboard installed as an App in Edge. I have worked out how to revoke the Google token when using it with Cloudflare Zero Trust, so I can now trigger the problem at will. |
|
Adding the reload as suggested by @arturv2000 fixes it. I will try and track down where it is failing. |
|
The exception is coming from I can't work out how to log the full response text, my attempts keep generating an exception. |
|
@cgjgh From what I can understand, the error is generated at this section: console.log(2)
// get the setup JSON from the server
const setup = await response.json()
setup.basePath = basePath
console.log(3)At least it only prints up to This was the best I Could do
|
|
I see what is happening. When the token has expired and the code does a fetch from which raises an exception in |
|
@cstns can you take a look at this, this week please, whilst I'm away? |
|
I have found another problem, which is possibly a completely different issue. Once the token has expired the dashboard continues to run ok. Presumably because existing connections continue to function. However, if one is using a non-core widget, then when one switches to the page containing the widget the console shows Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "text/html". Strict MIME type checking is enforced for module scripts per HTML spec. TypeError: Failed to fetch dynamically imported module: https://mydomain.org.uk/resources/@flowfuse/node-red-dashboard-2-ui-led/ui-led.umd.js I guess that those also get redirected to the login page Does it fetch those modules every time I switch to the page? |
To answer my own question, the answer is no, it fetches them the first time a page that needs them is opened, which makes sense. |
|
From what I can tell, the app is trying to boot up with data it receives after being redirected (due to various reasons) which in most cases are not valid json responses. I feel like we're just treating symptoms and not the root cause. My thought on this matter is, do we need to follow through with the ws connection and Vue app mount if the response we receive from the _setup call is not from our Node Red instance (assuming we got redirected to external auth providers) or we could safely force a redirect to Inputs are more than welcomed, I'll try to sort this out this week |
|
@cstns if you want to post suggested code to try I will happily give it a go. |
|
I created #944. I'm not completely sure if stopping the _setup process is enough, it may require a forcePageReload as well. I left a comment with todo to try both scenarios. |
|
@cstns Ok, I will give a try to check if with my current setup it solves the problem |
|
@cstns Don't know if supposed to answer here, or in the PR. With the changes in the PR, the problem continues:
It enters in the same "if" has indicated before. I added a console.log to the And it does not enter there. |
|
@arturv2000 Seems like you're using NodeRed auth which will not redirect and therefore not meet the conditions below and execute return or force reload... if (
response.url &&
typeof response.url === 'string' &&
!response.url.includes( new URL(window.location).origin)) {
// todo not sure if we should force redirect or just stop the setup process entirely
// forcePageReload('origins do not match')
return;
}Perhaps add a else if (response.status === 401) {
console.log('Unauthorized, reloading dashboard')
// todo not sure if we should force redirect or just stop the setup process entirely
// forcePageReload('origins do not match')
return;
}
Pretty sure you need to force reload otherwise it will react in same way as if not handling in Would Response.redirected be helpful here in detecting if not from origin URL? // Check if the response was redirected
if (response.redirected) {
console.log('Response was redirected to auth')
// handle reload
window.location.replace(window.location.origin + '/dashboard' + '?' + 'reloadTime=' + Date.now().toString() + Math.random())
return
} else {
console.log('Response came from NodeRed')
} |
|
My assumption was completely wrong. Based on your previous print screen I managed to figure out what's happening. This fetch API will be the end of me. Apparently a 404/401/403 http response code is not considered a failed request (which I was wrongly expecting it to be). More on this here. I made some changes so the redirect is triggered on 401 HTTP responses. Does anyone think we should force a refresh for other HTTP response codes as well? (ex 403) |
|
@cstns Seems like your latest changes (commit It requests the login information and loads the page correctly.
|
|
That's great news! I think we may have gotten to the bottom of it! We could leave the force redirect enabled only for 401 HTTP codes for the time being and update it if needed for other situations if they arise. I'll make some final touch-ups to the PR and submit it for review, I think we'll be able to merge it by EOD tomorrow if not surely this week. Release wise we'll have to wait for @joepavitt to return. Thank you all! |
|
Would wait for feedback from colinl since he has a different setup than mine regarding authentication. |
|
Yes, good point.
Is still open for debate |
|
I created #944. You can pull the |
|
Still failing with invalid json error. The console is showing a redirect to google login page as before. |
Or at least I would, but I can't work out how to do that, here it is with domain hidden. |
|
There's not much there because the response is truncated If you can confirm that the The question that would need to be sorted is if we should forcePageReload to the dashboard or the oauth provider page where the redirect was made. I hope we'll get an idea based on the |
|
I assume |
|
The origin is the dashboard url and response.url is the google auth page. redirected is true. By replacing I can see that it has successfully retrieved the google auth page. |
|
Replacing the I added a new case which will currently force a full page redirect to the redirected origin when host and response origins mismatch: case host.origin !== new URL(response.url).origin:
console.log('Following redirect:', response.url)
window.location.replace(response.url)
// forcePageReload('Redirected to different origin')
returnPlease give this setup a go, I think the full page redirect to the redirected origin and preventing any further app setup steps it will resolve the problem gracefully. If the redirect to the |
That was just so that I could log the text. The JSON.parse() still generated the exception as expected. It looks like you may have cracked it with the latest mod. Looking good so far :) |
|
Happy to hear! I'll mark the PR as ready to review! |
|
I have noticed one thing, but I don't know whether it is a PWA issue or something else. I updated a third party ui node (@colinl/node-red-dashboard-2-gauge-classic) to a new version, restarted node red, and adjusted the flows. The dashboard I had open on an Android device automatically picked up the modified flows but has not picked up the new version of the gauge node. Swiping down to make it refresh (which it does) does not pick it up, nor does closing the app and reopening it. Again, it may be relevant that this is a fairly old version of Android. |
|
It might be a PWA caching issue. Were you prompted in the dashboard at any time after updating (any) nodes to reload? Please open another issue for it and we can take a closer look. |
|
I wasn't prompted, but when I went to look at it a few minutes after updating it had automatically picked up the new flows. I will open an issue after doing a few more experiments to gain information. |
|
One aspect of the current behaviour that is not ideal is that if I have the dashboard open and the token expires then it keeps running for while, which is ok, but at some time later it pops up the Connection Lost notification and just sits there forever, presumably trying to reconnect and failing. It doesn't take me to the login screen at that point. To get going again I have to force a refresh, which then brings up the login screen. |
|
I have managed to catch some more information on this. Running on a laptop using Edge, some time after the token expires then there are a number of Then and it sits there showing the Connection Lost popup until I refresh the page. |
PWA only caches files that exist in the |
Yes, that was a bad assumption. @colinl does the same happen with a more generic catch all that would forcibly redirect to the dashboard? Please give it a test by leaving only the I assume this behavior is caused by something else because the _setup call is only called once when the application initially boots. Another thing that points in this direction would be the fact that you haven't included any text that would by caught by the setup catch block ('An error occurred:' or 'auth error:') |
|
Yes, it is odd, I don't know what is triggering the get that generates the CORS error. I have run it again, on Brave and this time the CORS error occurred before the connect failures. I wonder whether there is another, asynchronous, GET going on, completely unrelated to this code. |
Does that tell you anything? That appears to be ui/src/sw.js |
|
It is the hourly PWA check for updates in ui/src/components/PWABadge.vue. As such I suspect it can probably be ignored. |
|
That was my initial thought as well but I agree with @cgjgh, although the error might suggest that it's triggered by the worker, it's highly unlikely to be caused by it because it's just running the mounted vue app. I would turn my attention to the ws connection and socket.io in particular. As far as I can tell we don't have that many listeners for socket.io, and these errors you're experiencing would not get caught by the _setup call catch block. We could set up listeners for This would be out of scope for the task at hand, we should continue in another task for traceability. |
|
I have looked at the err object in the connect error fn |
|
The implementation would depend on what data the error we receive contains. If there's nothing relevant like in the case of fetch (response.ok, response.redirected or response.url) that we could use to deduce what went wrong, I'd say that using the existing forcePageReload would me more than enough. I'll be more than happy to assist along the way! |
|
I have posted issue #945 to cover the problem with third party ui nodes not reloading after update, and #946 to cover the whole area of recovery after a socket failure @arturv2000 could you post on #946 what symptom you see if you see if you have the dashboard open and then the token expires (if that situation does apply in your case). Perhaps your token never expires so it is not relevant. |
|
@cstns, looking at the code in |
|
It's not ideal in any way, I agree. When the time will come to implement custom paths for dashboards we'll need to address this. As we stand, we don't have a way of knowing what path the dashboard will be on so we just went with hardcoding it because we know it is static. One way of future proofing it would be to add it as a parameter in the |
|
I was forgetting that, of course, the client cannot access the ui-base. |
|
@arturv2000 are you still seeing a problem with this after the recent commits (to main, not released yet)? |
Current Behavior
Hello
This issue is related to #817 it seems that the fix applied in #881 is not sufficient, at least for Android.
Just updated one of the instances I have to v1.11.1 to check if this was fixed or not.
According to my tests:
In order for this error to show, is necessary to open the page in the browser (wait until the new PWA version is downloaded and the popup to reload appears), after this is necessary to close the browser window (all instances of chrome / edge / etc.., in windows is necessary to close the chrome icon on the notification tray and in android is necessary to force close the chrome browser), when trying to access again the site the page does not load until we force an incorrect address (like 'https://my.site.com/dashboar') at this point the login popup is shown and we can access the real address
https://my.site.com/dashboard.If we close the browser it is necessary to repeat the process.
The fix seems simple, according to my attempts is necessary to change the file
ui/src/main.mjsthecatchfor thefetch('_setup')needs to change to:It seems the original fix was enclosed in a series of
ifstatements that are not catching the correct error, since I just added the original fixwindow.location.replace....to theelseof the firstifExpected Behavior
No response
Steps To Reproduce
No response
Environment
Have you provided an initial effort estimate for this issue?
I am not a FlowFuse team member
The text was updated successfully, but these errors were encountered: