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

Add error page for initial REST request failure & Abort further load #1987

Merged
merged 9 commits into from
Sep 30, 2023

Conversation

florian-h05
Copy link
Contributor

Migitates and therefore closes #1205.

@florian-h05 florian-h05 requested a review from a team as a code owner July 24, 2023 13:51
@relativeci
Copy link

relativeci bot commented Jul 24, 2023

Job #1147: Bundle Size — 15.76MiB (~+0.01%).

5f44a35(current) vs 10e19e4 main#1146(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (2 changes)
                 Current
Job #1147
     Baseline
Job #1146
Initial JS 1.85MiB(~-0.01%) 1.85MiB
Initial CSS 608.95KiB 608.95KiB
Cache Invalidation 93.95% 93.81%
Chunks 217 217
Assets 683 683
Modules 1700 1700
Duplicate Modules 90 90
Duplicate Code 1.95% 1.95%
Packages 138 138
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #1147
     Baseline
Job #1146
CSS 859.49KiB 859.49KiB
Fonts 526.1KiB 526.1KiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.26MiB (~-0.01%) 9.26MiB
Media 295.6KiB 295.6KiB
Other 4.73MiB (~+0.01%) 4.73MiB

View job #1147 reportView main branch activity

@ghys
Copy link
Member

ghys commented Aug 19, 2023

Thanks - not a fan of alert() though.
For HABot I used the Quasar framework which had a special page provided that would be redirected to when these situations occurred (they would be accessible even when added as a PWA still, they could be a routable page or a pure static HTML page):

image

Note that it could be redesigned to offer more insight, it was just a simple version of something like this:

image

@florian-h05
Copy link
Contributor Author

Good idea, I will try to have a look at this.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Great! Let me know if you think it's better to merge this still, it's an improvement over a dialog that didn't show but alert() is IMO too obtrusive.

@florian-h05 florian-h05 changed the title Fix alert for initial REST request failure & Abort further load Add error page for initial REST request failure & Abort further load Aug 27, 2023
@florian-h05
Copy link
Contributor Author

@ghys I've added such a "page" (it is no real page, but instead part of app.vue, but anyway it should work fine):

image

@florian-h05 florian-h05 marked this pull request as draft August 27, 2023 13:20
@florian-h05 florian-h05 marked this pull request as ready for review August 27, 2023 13:33
@florian-h05
Copy link
Contributor Author

I have tested this on my dev system by deploying the UI artifact and then stopping the org.openhab.core.io.rest.core bundle, works fine!

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

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

Thanks, I like the different options for reloading at different levels!
In your last commit you reordered the buttons in a more logical manner (in increasing order of "strength") but also removed the warning on purging the cache, can you elaborate why? I thought it was rather nice to warn the user.

bundles/org.openhab.ui/web/src/components/app.vue Outdated Show resolved Hide resolved
@florian-h05
Copy link
Contributor Author

also removed the warning on purging the cache, can you elaborate why? I thought it was rather nice to warn the user.

Yeah, but when deploying the UI bundle to my dev server (instead of running the dev server) I noticed that the warning was not shown. I guess it is a CSS visibility problem, I will have to check that when I’m back home.

Signed-off-by: Florian Hotze <[email protected]>
@florian-h05
Copy link
Contributor Author

@ghys I have added the logic to only show the "Purge Caches And Reload" button from about.vue and re-introduced the confirmation dialog. It was not visible because of CSS visibility set on for f7-app, I fixed that and also cleaned up app.vue a bit. From my side, this is ready for merging now.

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Sep 11, 2023
@florian-h05 florian-h05 added this to the 4.1 milestone Sep 11, 2023
@florian-h05
Copy link
Contributor Author

@ghys I've fixed the conflict, can you please have a look?

@ghys ghys added the rebuild trigger a new Jenkins job label Sep 18, 2023
@ghys
Copy link
Member

ghys commented Sep 18, 2023

Uhh.. there are conflicts again. Probably due to me merging #2075. Could you please have a look again (sorry)?

I also think we should start to offload some methods out of app.vue to other files, it's becoming quite bulky - I mentioned it on another PR.

@florian-h05
Copy link
Contributor Author

Yeah, I've already read your comment on that PR. I will have a look at refactoring that error handling logic to a mixin, I will ping you when I am finished.

@florian-h05 florian-h05 removed the rebuild trigger a new Jenkins job label Sep 20, 2023
Signed-off-by: Florian Hotze <[email protected]>
@florian-h05
Copy link
Contributor Author

@ghys I have refactored this now: The reload stuff moved to the reload-mixin, which is now also used by about.vue and the connection health stuff from app.vue moved to the connection-health-mixin.

As I have done that refactoring from my iPad (I am currently on a short trip to a concert) I haven‘t been able to test this, so please test:

  • About page: „Reload“ and „Purge Service Worker and Caches“
  • app.vue: Whether the error page still completely works, and whether the SSE communication failure toast still works

@@ -0,0 +1,94 @@
import mixin from 'reload-mixin.js'
Copy link
Member

Choose a reason for hiding this comment

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

Thanks - this could have waited, enjoy your concert! (who is it?)
I'll tentatively do the testing tomorrow. The builds still fail, though:

[INFO] ERROR in ./src/components/connection-health-mixin.js
[INFO] Module not found: Error: Can't resolve 'reload-mixin.js' in '/home/runner/work/openhab-webui/openhab-webui/bundles/org.openhab.ui/web/src/components'
[INFO]  @ ./src/components/connection-health-mixin.js 1:0-36 3:11-16
Suggested change
import mixin from 'reload-mixin.js'
import mixin from './reload-mixin'

As an habit I dropped the .js extensions in imports, except in assets where is it required to build the component docs (as it is run by Node which requires them).

@florian-h05
Copy link
Contributor Author

Thanks - this could have waited, enjoy your concert! (who is it?)

We had to drive 6 hours though Germany, and I wasn't driving so I had plenty of free time.
Two Steps From Hell really performed an epic concert, absolutely great. I can really recommend checking out their music!

I'll tentatively do the testing tomorrow. The builds still fail, though:

I have just tested everything I mentioned above, fixed the build and addressed your review regading file extensions.

@ghys
Copy link
Member

ghys commented Sep 30, 2023

Two Steps From Hell really performed an epic concert, absolutely great. I can really recommend checking out their music!

This was what appeared on Spotify when I checked them out, so I guess it was already my kind of stuff :) I'm sure it was indeed epic.

image

I think we're good here!

@ghys ghys merged commit 7893bd9 into openhab:main Sep 30, 2023
4 checks passed
@florian-h05 florian-h05 deleted the load-failure branch September 30, 2023 19:58
florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Nov 22, 2023
This re-applies the fix from openhab#2075, which must have been lost while rebasing openhab#1987.

Signed-off-by: Florian Hotze <[email protected]>
florian-h05 added a commit that referenced this pull request Nov 22, 2023
This re-applies the fix from #2075, which must have been lost while
rebasing #1987.

Signed-off-by: Florian Hotze <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uncaught (in promise) TypeError: Cannot read properties of undefined (reading 'locale')
2 participants