hawtio / hawtio-next

Next generation Hawtio UI console
https://hawt.io
Apache License 2.0
7 stars 22 forks source link

Remote connection config uses wrong state #906

Closed grgrzybek closed 4 months ago

grgrzybek commented 5 months ago

When you manage remote connection (add, edit) we have some issues:

I have a fix for first problem and I have some suggestions:

This is a result of investigation related to hawtio/hawtio#3178 and #705

grgrzybek commented 4 months ago

@tadayosi @phantomjinx WDYT about adding a state to top bar for Hawtio instances showing remote Jolokia agents?

main Hawtio: image image

connect tab of Hawtio (connected and disconnected): image image

grgrzybek commented 4 months ago

BTW, in Hawtio 1.4 we had recent connections: image

tadayosi commented 4 months ago

@grgrzybek

in the top bar, when showing the remote tab, we may have a connection name with connection status, so user knows whether this tab is main or remote

Sounds great to me. The proposed change also looks great.

On the other hand, for the first part (and the original issue):

  • after adding a connection, when you edit it to change port, you see the port changed in the list, but if you open "edit" dialog again you see old port. close without saving, edit again and the port is correct
  • after changing name of the connection, existing browser tabs with this connection break

I think it's actually a corner case and the users wouldn't face the issue in most cases as I don't think the connection list is something the normal users would change so frequently.

That said, if we really want to fix it, I think it's ok to introduce the permanent ID instead of name, but then please make sure that the changes don't affect the import/export format of JSON and keep backward compatibility, because I assume it's something the users would share and maintain within their teams even since Hawtio v2.

Also, I'd suspect that the root cause of the issue is in the way the Connect plugin (and its service) is implemented. https://github.com/hawtio/hawtio-next/blob/main/packages/hawtio/src/plugins/connect/connections.ts The plugin is actually the initial prototype of the hawtio-next implementation, and I initially tested React's context & reducer combination (just like Redux) before reaching the current (more conservative) singleton services approach but then reached a conclusion that it's a bit of over engineering for the state management for Hawtio as the state in Hawtio mainly just comes from the Jolokia endpoint.

So I guess that refactoring the Connect to the singleton service approach in the same way as the rest of plugins should be an ideal way to fix the issue.

grgrzybek commented 4 months ago

@grgrzybek

in the top bar, when showing the remote tab, we may have a connection name with connection status, so user knows whether this tab is main or remote

Sounds great to me. The proposed change also looks great.

OK - it's quite easy, but to be fair, a little change is required to Plugin interface. Why?

If you think it's too invasive change, let me know. I can't think of any other way to create a plugin that doesn't have a left-side menu item and doesn't have a path, only header item contributions...

On the other hand, for the first part (and the original issue):

  • after adding a connection, when you edit it to change port, you see the port changed in the list, but if you open "edit" dialog again you see old port. close without saving, edit again and the port is correct
  • after changing name of the connection, existing browser tabs with this connection break

I think it's actually a corner case and the users wouldn't face the issue in most cases as I don't think the connection list is something the normal users would change so frequently.

That said, if we really want to fix it, I think it's ok to introduce the permanent ID instead of name, but then please make sure that the changes don't affect the import/export format of JSON and keep backward compatibility, because I assume it's something the users would share and maintain within their teams even since Hawtio v2.

I implemented "id" locally and everything works - I only didn't check import/export.

Also, I'd suspect that the root cause of the issue is in the way the Connect plugin (and its service) is implemented. https://github.com/hawtio/hawtio-next/blob/main/packages/hawtio/src/plugins/connect/connections.ts The plugin is actually the initial prototype of the hawtio-next implementation, and I initially tested React's context & reducer combination (just like Redux) before reaching the current (more conservative) singleton services approach but then reached a conclusion that it's a bit of over engineering for the state management for Hawtio as the state in Hawtio mainly just comes from the Jolokia endpoint.

So I guess that refactoring the Connect to the singleton service approach in the same way as the rest of plugins should be an ideal way to fix the issue.

Actually the problem is here: https://github.com/hawtio/hawtio-next/blob/9cd5b02/packages/hawtio/src/plugins/connect/remote/ConnectionModal.tsx#L115-L123

clear() is called for add and edit which calls setConnection(input) in both cases. The fix is to:

I checked and it works.

Anyway - before I send PR I'll verify few more things. The reason I started working on it is #705.

tadayosi commented 4 months ago

OK, let's fix it altogether as you propose.

grgrzybek commented 4 months ago

PR: #932