xh / hoist-react

🏗️ ⚛️ The XH Hoist toolkit for React
https://xh.io
Apache License 2.0
24 stars 9 forks source link

Admin cluster tab refresh #3783

Closed ghsolomon closed 3 weeks ago

ghsolomon commented 3 weeks ago

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be checked off to indicate they have been considered. If unclear if a step is relevant, please leave unchecked and note in comments.

If your change is still a WIP, please use the "Create draft pull request" option in the split button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to collapse multiple intermediate commits into a single commit representing the overall feature change. This helps keep the commit log clean and easy to scan across releases. PRs containing a single commit should be rebased when possible.

ghsolomon commented 3 weeks ago

New errorMessageBanner (local to the clusterTab component for now):

Screenshot 2024-09-11 at 2 14 18 PM

ghsolomon commented 3 weeks ago

@lbwexler switched to using errorMessage cmp:

Screenshot 2024-09-11 at 8 19 54 PM
amcclain commented 3 weeks ago

I'm going to let you guys decide which way you want to go on the error handling - but I can really picture myself trying to work with the admin client and looking at something specific in any of the cluster tabs to diagnose an issue and then 💥 everything just disappears.

I would rather go the other direction, with eg the log viewer not clearing itself so eagerly, but instead showing an indicator that you are looking at a view that couldn't be refreshed.

I suppose there are two ways to think about how to make a tool like this admin console reliable:

🤷

amcclain commented 3 weeks ago

(imagine it's the load balancer that's having the issue - every other request goes to a blackhole)

lbwexler commented 3 weeks ago

I think my proposal splits the difference -- it preserves maximal state, and just masks things temporarily.

We could eventually replace the mask with the banner, but I think my solution is less jarring

On Fri, Sep 13, 2024 at 10:36 AM Anselm McClain @.***> wrote:

I'm going to let you guys decide which way you want to go on the error handling - but I can really picture myself trying to work with the admin client and looking at something specific in any of the cluster tabs to diagnose an issue and then 💥 everything just disappears.

I would rather go the other direction, with eg the log viewer not clearing itself so eagerly, but instead showing an indicator that you are looking at a view that couldn't be refreshed.

I suppose there are two ways to think about how to make a tool like this admin console reliable:

  • Never show anything that might be out of date - if in doubt, clear and hide it all
  • Always strive to show/maintain what you can - something is going wrong or flapping in / out, let an admin review / keep reviewing whatever we can keep showing

🤷

— Reply to this email directly, view it on GitHub https://github.com/xh/hoist-react/pull/3783#issuecomment-2349113298, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARTL2PB5WQBSGOZMU7MTGDZWLZ67AVCNFSM6AAAAABOBRCWY2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBZGEYTGMRZHA . You are receiving this because you were mentioned.Message ID: @.***>