UI: Redesign/organize settings page

[chakflying]

As mentioned, the settings page is rapidly getting cluttered with different sections. The Save button is also in an awkward place as only some settings needs to be manually saved. I think a better design would be to have sections on the left side, and the main pane shows settings of this section only. The "Save" button can then appear only if that page needs to be saved.

[louislam]

I personally prefer to always show the monitor list, what do you think if implemented like this:

[chakflying]

I think it works for GitHub because there is a clear hierarchy of "most of the time you only need code, issues, and PRs" and they can just hide the rest as the screen width changes. But we don't really have that, and I also rarely see settings sub-pages get separated into tabs like that. How about we split the settings pane itself then?

[louislam]

I think it works for GitHub because there is a clear hierarchy of "most of the time you only need code, issues, and PRs" and they can just hide the rest as the screen width changes. But we don't really have that, and I also rarely see settings sub-pages get separated into tabs like that. How about we split the settings pane itself then?

This is unexpectedly better looking than I thought. Let's take this way.

[deefdragon]

I personally see UK as about 3 key features, the monitoring, the notifications, and the status pages, and I am wondering if notifications should get their own section like the dashboard. Should #233 et. all, be accounted for and space prepared ahead of time while the settings are getting updated? Or should that be delegated to a separate PR? (I assume the latter, but worth bringing up.)

[chakflying]

Are you talking about adding a completely new page to manage notifications settings? IMO a lot of them are and will be "per monitor" rather than global, so I don't see the need. Also, the status page currently manages its own settings. It would be debatable if in the future "multiple status pages" becomes a thing, the settings page be a place to manage it. But I'm also not sure. But this refactor would at least convert each settings sub-section to its own component, so future expansion would be easier and more organized.

[louislam]

@chakflying's implementation is preferred.

[chakflying]

@louislam still working through this, but using $parent.$parent.settings already seem like a phenomenally bad idea. Do you think each settings sub-page should manage its own state instead?

[louislam]

@louislam still working through this, but using $parent.$parent.settings already seem like a phenomenally bad idea. Do you think each settings sub-page should manage its own state instead?

I think we can put into a function that returns $parent.$parent.settings in this stage.

[chakflying]

Tested the tests work locally, but it fails on GitHub for seemingly no reason...

[louislam]

Tested the tests work locally, but it fails on GitHub for seemingly no reason...

I had similar experience, I found that jest + puppeteer is really hard to use especially on GitHub Action, because their machine is too slow.

[chakflying]

I think it's finally fixed.

[chakflying]

@louislam I think I found a bug while writing tests that "Disable Auth" currently breaks the settings page because of #772. jwtDecode would fail since the token is set to autoLogin. I can add a fix here, or if you think you won't merge this soon we can add a separate fix first.

[louislam]

@louislam I think I found a bug while writing tests that "Disable Auth" currently breaks the settings page because of #772. jwtDecode would fail since the token is set to autoLogin. I can add a fix here, or if you think you won't merge this soon we can add a separate fix first.

Ops, does it crash the whole setting page? If yes, it would be great if we fix it first

[louislam]

Fixed in 95bae8289d3a391065e66673cd969c63b7101ef2

[louislam]

Just found one issue, it seems that there is no way to save this value.

[chakflying]

Oops that was dumb, I added a save button in the monitor history page.

[louislam]

If you cannot make the unit test working again, just let it go. I can test it manually.

I honestly hate jest + puppeteer now, it is not reliable and hard to write at all in my opinion. May look for an alternative and consider to drop this.

[chakflying]

Thanks to a very recent StackOverflow post, I finally got the error message to show up:

 error: [Error: TypeError: Cannot read properties of undefined (reading 'name')
      at Socket.<anonymous> (http://127.0.0.1:3002/assets/index.714bebe6.js:7211:66)
      ...

I traced this to socket.js:126, which is a toast.success() trying to show the first beat. I think we actually found a race-condition that can happen in slow machines, like the poor github actions servers:

1. The client goes to /add, input a new monitor, click "Submit"

2. The client submit() sends add through the WebSocket

3. On the server, receiving add, it then:

   • store the monitor to DB
   • start the monitor
   • send the new monitor list

4. Right after the monitor is started, if the monitor target responds very quickly, it's possible that the client will receive the first beat before it receives the new monitor list!

I switched around the order in server.js:544 so it sends the monitor list first before starting the monitor, and all the tests magically passed. I think I'll clean up the code tomorrow, remove all the unnecessary changes and confirm that this is now working.

[chakflying]

Welll, I really don't know about this one. I disabled the clear statistics test for now.