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

X-Admin app improvements #761

Merged
merged 28 commits into from
Aug 19, 2024
Merged

X-Admin app improvements #761

merged 28 commits into from
Aug 19, 2024

Conversation

Velua
Copy link
Contributor

@Velua Velua commented Jun 19, 2024

  • Remove in house hash routing, adopt React Router Hash Routing.
  • Remove custom hooks for queries and adopt React Query.
  • Zod type API responses / Centralised all chain API requests.
  • Add new dashboard page alongside graphana
  • Remove old boot page / form
  • Add setup / Join wizard

@Velua Velua self-assigned this Jun 19, 2024
@Velua Velua linked an issue Jul 16, 2024 that may be closed by this pull request
@Velua Velua marked this pull request as ready for review August 5, 2024 04:50
@James-Mart
Copy link
Member

Locally, I merged with the latest main, built and tested this. But I'm getting this ZodError on validating the http_timeout field of the config:
image

If I change the zod schema to expect a string then it works and lets me boot.

@James-Mart James-Mart self-requested a review August 5, 2024 14:58
@James-Mart
Copy link
Member

This looks very awesome

Copy link
Member

@James-Mart James-Mart left a comment

Choose a reason for hiding this comment

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

I left various feedback on this issue that I gathered while using the interface. Nothing I'd block this PR over.

Only thing that seems necessary to fix before merge (from a usability perspective) is the zod parse error that prevents the chain from booting.

@James-Mart James-Mart changed the title adopt hash router and react query X-Admin app improvements Aug 5, 2024
@James-Mart James-Mart added the System app Related to system services and their apps/plugins label Aug 5, 2024
@Velua Velua requested a review from James-Mart August 8, 2024 09:05
@James-Mart
Copy link
Member

Arrow button covered by expanded Advanced tab
image

@James-Mart
Copy link
Member

James-Mart commented Aug 8, 2024

--EDIT--
This was a bug in native, fixed here

My config seems to get corrupted at some point... a "service" key is getting deleted somehow
image

This happening to you?

@James-Mart
Copy link
Member

James-Mart commented Aug 8, 2024

--EDIT--
This was a bug in native, fixed here

Also if I start out with http_timeout = 4s, I notice later in my config it is saying
http-timeout = 4000 s

Is it converting to ms and using the wrong label?

@James-Mart
Copy link
Member

James-Mart commented Aug 8, 2024

If I add a peer, and then I click "disconnect", I think there's two things that are strange:

  1. After a while, the connection is automatically re-established. This is unsurprising because presumably the peer is still in the configured list of peers, but the UX feels strange. I think we can probably improve this UX later.
  2. If I disconnect, and then click the "..." button, the option still says "Disconnect" instead of "Connect"
    image

@swatanabe
Copy link
Collaborator

Also if I start out with http_timeout = 4s, I notice later in my config it is saying http-timeout = 4000 s

Is it converting to ms and using the wrong label?

If that's happening, it's probably a bug on the native side. The /native/admin/config endpoint takes an integer in microseconds, with no units.

@Velua
Copy link
Contributor Author

Velua commented Aug 9, 2024

If I add a peer, and then I click "disconnect", I think there's two things that are strange:

1. After a while, the connection is automatically re-established. This is unsurprising because presumably the peer is still in the configured list of peers, but the UX feels strange. I think we can probably improve this UX later.

2. If I disconnect, and then click the "..." button, the option still says "Disconnect" instead of "Connect"
   ![image](https://private-user-images.githubusercontent.com/42752296/356286608-909213d2-c1a6-45e2-a365-90c0886cb2fe.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjMxNzA5ODMsIm5iZiI6MTcyMzE3MDY4MywicGF0aCI6Ii80Mjc1MjI5Ni8zNTYyODY2MDgtOTA5MjEzZDItYzFhNi00NWUyLWEzNjUtOTBjMDg4NmNiMmZlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODA5VDAyMzEyM1omWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTRjOGIzNjhiYTY4YmY2YTdmZjFhMmZhOGUzNWFlOTJhNjc0MzUxZjViNGExMmVjNTc2ZGZiNjJkZDAyNGI4NjkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.7xUucIzEqMgB-pYhvtIpDhqNraUQu_4uJPoJCKAeuJw)

Yeah I think the solution here is just to prevent crossing lanes,

  • Transient connections get disconnect
  • Persistent connections get remove

It's awkward if persistent connections get both only to reconnect again anyway.

  • Render "disconnect" button only on active transient connections.
  • Render "remove" only on online & offline persistent connections.

@swatanabe
Copy link
Collaborator

My config seems to get corrupted at some point... a "service" key is getting deleted somehow

It's a native bug that seems to be caused by having multiple entries for localhost

@James-Mart
Copy link
Member

Transient connections get disconnect

Although why even bother disconnecting if it's a transient connection, since it'll just come back again, right? Unless we can blacklist somehow @swatanabe

Comment on lines -706 to +707
OUTPUT ${SERVICE_DIR}/Default.psi
NAME Default
OUTPUT ${SERVICE_DIR}/DevDefault.psi
NAME DevDefault
Copy link
Collaborator

Choose a reason for hiding this comment

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

psibase boot needs to be updated

Copy link
Member

Choose a reason for hiding this comment

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

Tester as well, yeah?

let default_services = vec!["Default".to_string()];

and
let default_services: Vec<String> = vec!["Default".to_string()];

@Velua Velua merged commit 5c5e7e3 into main Aug 19, 2024
4 checks passed
@Velua Velua deleted the x-admin-wizard branch August 19, 2024 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
System app Related to system services and their apps/plugins
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Admin app rework
3 participants